get_pmg_structure returns magnetic moments under site_properties key "magnetic_moments" instead of "magmom"

JonathanSchmidt1 commented 7 months ago

Python version

3.9.13

Pymatgen version

2024.2.23

Operating system version

No response

Current behavior

Recently the get_pmg_structure and the get_phonopy_structure methods in pymatgen/io/phonopy.py were updated to include magnetic moments. ( https://github.com/materialsproject/pymatgen/pull/3555 ) the get_pmg_structure method should have the magnetic moment under the magmom key instead it is magnetic moments. I also overlooked this in the pull request ...

    return Structure(
        lattice,
        symbols,
        frac_coords,
        site_properties={"phonopy_masses": masses, "magnetic_moments": magmoms},
    )

This leads to problems e.g. when automatically creating incars in atomate2 etc. (working on adding magmoms to the phonon workflow @JaGeo )

Expected Behavior

It should be under magmom:

 return Structure(
        lattice,
        symbols,
        frac_coords,
        site_properties={"phonopy_masses": masses, "magmom": magmoms},
    )

than it also corresponds to the other method that converts from pymatgen to phonopy:

   return PhonopyAtoms(
        symbols=symbols,
        cell=pmg_structure.lattice.matrix,
        scaled_positions=pmg_structure.frac_coords,
        magnetic_moments=pmg_structure.site_properties.get("magmom"),
    )

The unit test would also need to be changed to confirm that the site_properties are the same. Right now it only tests the following for magmom: assert list(struct_ph.magnetic_moments) == magmoms

maybe it would be possible to add: assert struct_pmg_round_trip.site_properties['magmom']==struct_pmg.site_properties['magmom']

Minimal example

from pymatgen.io.phonopy import get_pmg_structure, get_phonopy_structure
from pymatgen.core import Structure, Lattice

lattice_vectors = [
    [2.8126969436818903, 0.0000000000000000, 0.0000000000000000],
    [0.0000000000000000, 2.8126969436818903, 0.0000000000000000],
    [0.0000000000000000, 0.0000000000000000, 2.8126969436818903],
]
lattice = Lattice(lattice_vectors)

species = ["Cr", "Cr"]
positions = [
    [0.0000000000000000, 0.0000000000000000, 0.0000000000000000],  # First Cr atom
    [0.5000000000000000, 0.5000000000000000, 0.5000000000000000],  # Second Cr atom
]
cr_structure = Structure(lattice, species, positions, coords_are_cartesian=False, site_properties={"magmom": [5, -5]})
print(get_pmg_structure(get_phonopy_structure(cr_structure)).site_properties)

Relevant files to reproduce this bug

No response

Andrew-S-Rosen commented 7 months ago

Thanks for the report, @JonathanSchmidt1. You are absolutely right that this is a bug that should be fixed. Are you interested in submitting a PR for this patch?

CCing @tomdemeyere to let him know about the bug in case it is relevant to his work.

JaGeo commented 7 months ago

@JonathanSchmidt1 yes, please go ahead and submit a bug fix if you like to. As previously mentioned, we have never used pymatgen or atomate2 for phonon runs of magnetic materials.

materialsproject / pymatgen