Fix from_ase_atoms for Molecule

[DanielYang59]

Summary

• Fix from_ase_atoms for Molecule, to close Molecule.from_ase_atoms() does not pass kwargs appropriately #4320
• Add tests

[DanielYang59]

Need careful check on this: AFAIK there is no other concrete class other than IMolecule/IStructure of the abstract base classSiteCollection that would reply on inheriting from_ase_atoms?

[Andrew-S-Rosen]

I don't believe there are. Even if there were, then presumably we would currently be missing out on such instances anyway since the pre-existing methods were on Structure and Molecule, right?

[Andrew-S-Rosen]

Is there a problem here in that it will produce an IStructure rather than a Structure?

[DanielYang59]

the pre-existing methods were on Structure and Molecule, right?

If I understand the question correctly, current defination of from_ase_atoms is given by the abstract base case SiteCollection (from which IStructure/IMolecule inherit):

pymatgen/src/pymatgen/core/structure.py

Lines 998 to 1009 in 55d0934

	def from_ase_atoms(self, **kwargs) -> Structure:
	"""Convert ase.Atoms to pymatgen Structure.
	Args:
	kwargs: Passed to AseAtomsAdaptor.get_structure.
	Returns:
	Structure
	"""
	from pymatgen.io.ase import AseAtomsAdaptor
	return AseAtomsAdaptor.get_structure(self, **kwargs)

---

Is there a problem here in that it will produce an IStructure rather than a Structure?

Thanks for catching this, this method should give the mutable Structure/Molecule by default, I would fix the type annotation:

pymatgen/src/pymatgen/io/ase.py

Line 233 in 55d0934

def get_structure(atoms: Atoms, cls: type[Structure] = Structure, **cls_kwargs) -> Structure:

pymatgen/src/pymatgen/io/ase.py

Lines 307 to 319 in 55d0934

	# Return a Molecule object if that was specifically requested;
	# otherwise return a Structure object as expected
	if cls == Molecule:
	structure = cls(symbols, positions, properties=properties, **cls_kwargs)
	else:
	structure = cls(
	Lattice(lattice, pbc=atoms.pbc),
	symbols,
	positions,
	coords_are_cartesian=True,
	properties=properties,
	**cls_kwargs,
	)

Perhaps we should give the corresponding class instead (i.e. IStructure.from_ase_atoms should give IStructure instead of Structure, the same for IMolecule), which would be less confusing?