Implement mapping parser for phonopy by ladinesa · Pull Request #264 · FAIRmat-NFDI/nomad-parser-plugins-simulation

ladinesa · 2026-10-08T09:21:31Z

Write phonopy properties to custom phonopy schema.
Use mapping parser for phonopy
Make fhiaims use also mapping for phonon archive

Copilot

🟡 Changes recommended

Calculator-dependent unit errors and several parser failure paths can produce inaccurate results or abort valid partial parses.

8 open findings

Convert force constants from calculator units to eV per Å² · New
Use calculator-specific band frequency conversion factors · New
Convert legacy force constants from native calculator units · New
Avoid cached parser state across data objects · New
Preserve first-atom displacements for type-2 arrays · New
Handle missing force datasets without failing archive conversion · New
Guard each referenced output list before indexing · New
Map output references declaratively instead of mutating archives · New

What changed in this PR

Implements annotation-driven Phonopy parsing into the simulation schema.

Changes:

Adds mappings for systems, methods, force constants, bands, DOS, and thermodynamics.
Introduces a mapping-based archive writer.
Expands unit and integration coverage.

File	Description
`src/nomad_simulation_parsers/parsers/phonopy/parser.py`	Adds mapping transforms and output generation.
`src/nomad_simulation_parsers/schema_packages/phonopy.py`	Defines Phonopy schema mappings.
`tests/parsers/phonopy/test_mapping.py`	Tests mapping behavior.
`tests/parsers/phonopy/test_recognition.py`	Tests YAML loading and cached transforms.
`tests/parsers/phonopy/test_integration.py`	Verifies mapped archive outputs.
`tests/parsers/phonopy/test_archive_writer.py`	Extends legacy writer tests.
`tests/parsers/phonopy/conftest.py`	Routes fixtures through the public parser.
`tests/parsers/phonopy/README.md`	Updates the test matrix.

🧠 Review effort: Balanced

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

VonaCecilia · 2026-10-09T14:13:36Z

+        if force_constants is None:
+            obj.produce_force_constants()
+            force_constants = obj.force_constants
+        return [{'value': force_constants * ureg.eV / ureg.angstrom**2}]


Phonopy documentation

+                'n_bands': segment['frequencies'].shape[-1],
+                'value': segment['frequencies'] * ureg.joule,
+                'endpoints_labels': [str(label) for label in segment['labels']],


        force_constants = phonopy_obj.force_constants
-        force_constants = (
-            (force_constants * ureg.eV / ureg.angstrom**2).to('J/(m**2)').magnitude
-        )
+        if force_constants is None:
+            phonopy_obj.produce_force_constants()
+            force_constants = phonopy_obj.force_constants
+        force_constants = force_constants * ureg.eV / ureg.angstrom**2


ladinesa · 2026-10-18T12:40:06Z

+    _phonon_properties: PhononProperties | None = None
+    _thermodynamic_properties: dict[str, list[dict[str, Any]]] | None = None


This is ok since assigning a new data object will load the file again.

+        force_constants = obj.force_constants
+        if force_constants is None:
+            obj.produce_force_constants()
+            force_constants = obj.force_constants
+        return [{'value': force_constants * ureg.eV / ureg.angstrom**2}]


ladinesa · 2026-10-19T10:05:32Z

+        if self.archive.data.outputs:
+            self.archive.data.outputs[
+                0
+            ].model_system_ref = self.archive.data.model_system[-1]
+            self.archive.data.outputs[
+                0
+            ].model_method_ref = self.archive.data.model_method[0]


swtiched to mapping

ladinesa · 2026-10-19T10:05:58Z

+        # References are intentionally linked after conversion: the generic
+        # mapper cannot resolve object references between freshly-created list
+        # elements from a plain source dictionary.


switched to mapping

VonaCecilia

The PR looks overall complete and well structure. I have owever a few clarification to ask:

is there a particular reason why some of these quantities and sections are defined specifically in the Phonopy plugin rather than in the general nomad-simulations schema? For example, properties like force constants or phonon band structures could also be relevant to other parsers. Is there a plan to move them to the general schema later, or to reuse existing components where possible?
Do we plan to account for the fact that the units of quantities in Phonopy output files can vary depending on the calculator used and read them from the output where they are specified? In general we should be carefull with the unit in this PR.
For what I see the tests do not cover some end-to-end quantities like frequency of the dos, temperrature, etc. In my local test they are not populated properly, is it possible to verify it?
I did not go deep into calculator.py since it is not part of the PR, however I have notice that we need to run some phonopy script to get properties like the thermal one, and there are some paramters to set that determine the accuracy of the results. When the file are not present, do we get them usining the Phonopy API, right? how do these parameters are considered?

VonaCecilia · 2026-10-09T12:39:58Z

This is not directly related to the PR, but I think we should verify the normalization of the thermodynamic properties.

According to the Phonopy documentation, free energy is reported in kJ/mol and heat capacity in J/K/mol, where one mole refers to Avogadro's number of primitive cells.

Since we want the total free energy in J and the total heat capacity in J/K, both quantities should be scaled accordingly. The conversion to eV and eV/K per primitive cell is already performed in get_thermodynamical_properties(), so the remaining conversions should be:

scale = n_atoms_supercell / n_atoms_primitive
free_energy = free_energy * scale * eV_to_J
heat_capacity = heat_capacity * scale * eV_to_J

Currently, different scaling factors are used for free energy and heat capacity.

Also, I would rename n_atoms to n_atoms_unitcell for clarity, since it refers to the input unit cell rather than necessarily the primitive cell.

VonaCecilia · 2026-10-09T13:47:51Z

+                ],
+                'representations': [
+                    {
+                        'lattice_vectors': atoms.cell * ureg.angstrom,


According to the Phonopy documentation, the length units depend on the calculator used to generate the input files. Therefore, the assumption that positions and lattice vectors are expressed in Ångström is only correct for certain calculators, such as VASP, CRYSTAL, and CP2K. Is the conversion to Ångström performed elsewhere?

The same for the rest of the Ångström units below.

Could we determine the units once when loading the file and reuse them throughout the parser, rather than hardcoding the units in each transformer? The YAML file already provides unit information, and this would make the parser more consistent across different calculators. We should also check which conversions are already performed by phonopy.load().

I always thought that phonopy is working on ase atoms objects, I will check,

VonaCecilia · 2026-10-09T14:07:05Z

+        return [
+            {
+                'name': 'harmonic lattice dynamics',
+                'type': 'finite displacement',


Maybe we could rename this method to get_finite_displacement_method() to make its purpose more explicit, since it specifically creates the dictionary for a finite-displacement calculation.

VonaCecilia · 2026-10-09T14:13:36Z

+        if force_constants is None:
+            obj.produce_force_constants()
+            force_constants = obj.force_constants
+        return [{'value': force_constants * ureg.eV / ureg.angstrom**2}]


Phonopy documentation

VonaCecilia · 2026-10-12T09:35:45Z

    return sec_system


+def _populate_bandstructures(


why do we have both this and get_phonon_band_structure? Is it needed to maintain some older part of the code?

yes it is the # 3 in TODO: fhiaims still using the old api

VonaCecilia · 2026-10-16T08:46:16Z

+class QPath(Variables):
+    """Reciprocal-space path points in fractional coordinates."""
+
+    points = Quantity(type=np.float64, shape=['n_points', 3])
+    add_mapping_annotation(points, PHONOPY_KEY, '.points')
+
+
+class PhononBandStructure(PhysicalProperty):


Would it make sense to align this schema more closely with the existing ElectronicBandStructure in nomad-simulations? Both represent band values along reciprocal-space paths and have similar requirements. Using a consistent structure for the paths, labels, and values could simplify plotting and make the data easier to work with across different parsers.

yes absolutely, would it be possible for you to work on the phonon schema in nomad-simulations in parallel? The I can make the switch also in this pr.

I think it's best to discuss this in the next area C meeting!!

VonaCecilia · 2026-10-16T09:45:05Z

+    monkeypatch.setattr(
+        parser_module, 'PhononProperties', lambda *args, **kwargs: properties
+    )
+    monkeypatch.setattr(parser_module, 'get_bandstructures', lambda _: [])


Is it intentional that get_bandstructures() returns an empty list here?

VonaCecilia · 2026-10-16T10:07:10Z

+    assert len(archive.data.outputs) == 1
+    assert archive.data.outputs[0].model_method_ref is archive.data.model_method[0]
+    assert archive.data.outputs[0].force_constants[0].value.shape == (2, 2, 3, 3)
+    assert len(archive.data.outputs[0].phonon_dos) == 1


I would also add assertions for the DOS quantities to verify that the frequencies are correctly populated in the archive.

VonaCecilia · 2026-10-16T10:08:49Z

 @pytest.mark.integration
-def test_cp2k_fixture_band_segments_for_archive_writer(cp2k_phonopy_object):
-    properties = PhononProperties(cp2k_phonopy_object, get_logger(__name__), k_mesh=2)
+def test_cp2k_fixture_band_segments_for_archive_writer(


This test only verifies the output of get_bandstructure(), but not whether the band structure data is correctly populated in the NOMAD archive, right?

VonaCecilia · 2026-10-16T10:14:46Z

+        assert len(outputs.phonon_band_structures) == 9
+        assert len(outputs.phonon_dos) == 1
+        assert len(outputs.vibrational_free_energies) == 1
+        assert outputs.model_system_ref is simulation.model_system[1]


Would it make sense to also check the heat capacity, temperature points, and band structure and DOS quantities? This would help verify that the data is actually populated in the archive. At least, I would add an assertion for the heat capacity, since we already check the free energy.

ladinesa · 2026-10-17T10:07:01Z

The PR looks overall complete and well structure. I have owever a few clarification to ask:

* is there a particular reason why some of these quantities and sections are defined specifically in the Phonopy plugin rather than in the general nomad-simulations schema? For example, properties like force constants or phonon band structures could also be relevant to other parsers. Is there a plan to move them to the general schema later, or to reuse existing components where possible?

* Do we plan to account for the fact that the units of quantities in Phonopy output files can vary depending on the calculator used and read them from the output where they are specified? In general we should be carefull with the unit in this PR.

* For what I see the tests do not cover some end-to-end quantities like frequency of the dos, temperrature, etc. In my local test they are not populated properly, is it possible to verify it?

* I did not go deep into calculator.py since it is not part of the PR, however I have notice that we need to run some phonopy script to get properties like the thermal one, and there are some paramters to set that determine the accuracy of the results. When the file are not present, do we get them usining the Phonopy API, right? how do these parameters are considered?

This is just a transient fix while waiting for the definition to be formalized in nomad-simulations.
Yes, I agree will go through all units to verify this.
I will add tests for these.
The calculator only needs a phonopy obj, which for a phonopy yaml file is read via the phonopy reader together with the force constants file, and for fhiaims is constructed by reading the forces and parameters from the fhiaims outputs. There are no parameters injected by the parser.

Implement mapping parser for phonopy

4e2cf9c

ladinesa requested a balanced review from Copilot October 8, 2026 09:21

Copilot started reviewing on behalf of ladinesa October 8, 2026 09:22

View session

ladinesa marked this pull request as draft October 8, 2026 09:24

Copilot AI reviewed Oct 8, 2026

View reviewed changes

JFRudzinski requested a review from VonaCecilia October 8, 2026 11:46

VonaCecilia reviewed Oct 16, 2026

View reviewed changes

ladinesa added 6 commits October 19, 2026 12:34

Address review

0a9d0a4

Use mapping parser for fhiaims

c91d31c

Add tests

e90099f

Common phonon workflow util

2d50751

Add phonopy workflow parsing in vasp

7d6861c

Fixes

825c114

		_phonon_properties: PhononProperties \| None = None
		_thermodynamic_properties: dict[str, list[dict[str, Any]]] \| None = None

Conversation

ladinesa commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VonaCecilia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ladinesa commented Oct 17, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ladinesa commented Oct 8, 2026 •

edited

Loading