Fix generate_phonon_displacements magmom removal side effect (#1064)

* fix lints and typos

* fix generate_phonon_displacements side effect: removes magmoms of passed structure in-place

* fix ruff PYI063 Use PEP 570 syntax for positional-only parameters

Author: janosh

.pre-commit-config.yaml

@@ -3,7 +3,7 @@ default_language_version:
 exclude: ^(.github/|tests/test_data/abinit/)
 repos:
 - repo: https://github.com/charliermarsh/ruff-pre-commit
-  rev: v0.7.3
+  rev: v0.8.0
   hooks:
   - id: ruff
     args: [--fix]
@@ -45,7 +45,7 @@ repos:
     args: [--ignore-words-list, 'titel,statics,ba,nd,te,atomate']
     types_or: [python, rst, markdown]
 - repo: https://github.com/kynan/nbstripout
-  rev: 0.8.0
+  rev: 0.8.1
   hooks:
     - id: nbstripout
       args:

pyproject.toml

@@ -207,7 +207,6 @@ ignore = [
     "PLR0913", # too many arguments
     "PLR0915", # too many local statements
     "PLR2004",
-    "PT004",   # pytest-missing-fixture-name-underscore
     "PT006",   # pytest-parametrize-names-wrong-type
     "PT013",   # pytest-incorrect-pytest-import
     "PTH",     # prefer Pathlib to os.path

src/atomate2/ase/schemas.py

@@ -99,7 +99,7 @@ class AseBaseModel(BaseModel):
     )
     molecule: Optional[Molecule] = Field(None, description="The molecule at this step.")
 
-    def model_post_init(self, __context: Any) -> None:
+    def model_post_init(self, _context: Any) -> None:
         """Establish alias to structure and molecule fields."""
         if self.structure is None and isinstance(self.mol_or_struct, Structure):
             self.structure = self.mol_or_struct

src/atomate2/common/flows/qha.py

@@ -37,7 +37,7 @@ class CommonQhaMaker(Maker, ABC):
     Then we scale the relaxed structure, and
     then compute harmonic phonons for each scaled
     structure with Phonopy.
-    Finally, we compute the Gibb's free energy and
+    Finally, we compute the Gibbs free energy and
     other thermodynamic properties available from
     the quasi-harmonic approximation.
 

src/atomate2/common/jobs/electrode.py

@@ -250,17 +250,17 @@ def get_relaxed_job_summaries(
     """
     relax_jobs = []
     outputs = []
-    for ii, structure in enumerate(structures):
+    for idx, structure in enumerate(structures):
         job_ = relax_maker.make(structure=structure)
         relax_jobs.append(job_)
-        job_.append_name(f" {append_name} ({ii})")
-        d_ = {
-            "structure": job_.output.structure,
-            "entry": job_.output.entry,
-            "dir_name": job_.output.dir_name,
-            "uuid": job_.output.uuid,
-        }
-        outputs.append(RelaxJobSummary(**d_))
+        job_.append_name(f" {append_name} ({idx})")
+        job_summary = RelaxJobSummary(
+            structure=job_.output.structure,
+            entry=job_.output.entry,
+            dir_name=job_.output.dir_name,
+            uuid=job_.output.uuid,
+        )
+        outputs.append(job_summary)
 
     replace_flow = Flow(relax_jobs, output=outputs)
     return Response(replace=replace_flow, output=outputs)

src/atomate2/common/jobs/phonons.py

@@ -122,17 +122,23 @@ def generate_phonon_displacements(
         scheme to generate kpath
     code:
         code to perform the computations
+
+    Returns
+    -------
+    List[Structure]
+        Displaced structures
     """
     warnings.warn(
         "Initial magnetic moments will not be considered for the determination "
         "of the symmetry of the structure and thus will be removed now.",
-        stacklevel=1,
-    )
-    cell = get_phonopy_structure(
-        structure.remove_site_property(property_name="magmom")
-        if "magmom" in structure.site_properties
-        else structure
+        stacklevel=2,
     )
+    if "magmom" in structure.site_properties:
+        # remove_site_property is in-place so make a structure copy first
+        no_mag_struct = structure.copy().remove_site_property(property_name="magmom")
+    else:
+        no_mag_struct = structure
+    cell = get_phonopy_structure(no_mag_struct)
     factor = get_factor(code)
 
     # a bit of code repetition here as I currently

src/atomate2/forcefields/schemas.py

@@ -22,7 +22,7 @@ class ForcefieldResult(AseResult):
         None, description="The structure in the final trajectory frame."
     )
 
-    def model_post_init(self, __context: Any) -> None:
+    def model_post_init(self, _context: Any) -> None:
         """Populate final_structure attr."""
         self.final_structure = getattr(
             self, "final_structure", self.final_mol_or_struct

tests/vasp/test_sets.py

@@ -52,7 +52,7 @@ def test_user_incar_settings():
 
     # check to see if user incar settings (even when set to nonsensical values, as done
     # below) are always preserved.
-    uis = {
+    user_settings = {
         "ALGO": "VeryFast",
         "EDIFF": 1e-30,
         "EDIFFG": -1e-10,
@@ -80,16 +80,16 @@ def test_user_incar_settings():
         "LDAUTYPE": 2,
     }
 
-    static_set_generator = StaticSetGenerator(user_incar_settings=uis)
+    static_set_generator = StaticSetGenerator(user_incar_settings=user_settings)
     incar = static_set_generator.get_input_set(structure, potcar_spec=True)["INCAR"]
 
-    for key in uis:
+    for key, val in user_settings.items():
         if isinstance(incar[key], str):
-            assert incar[key].lower() == uis[key].lower()
-        elif isinstance(uis[key], dict):
-            assert incar[key] == [uis[key][str(site.specie)] for site in structure]
+            assert incar[key].lower() == val.lower()
+        elif isinstance(val, dict):
+            assert incar[key] == [val[str(site.specie)] for site in structure]
         else:
-            assert incar[key] == uis[key]
+            assert incar[key] == val
 
 
 @pytest.mark.parametrize(