Migration of fundamental system and electronic properties for normalization to results#170
Merged
Conversation
JFRudzinski
force-pushed
the
test-data-normalization
branch
from
5d5cfe7 to
f1076d7
Compare
…nto nomad-simulations mappings for results population
ladinesa
reviewed
Jul 6, 2026
ladinesa
reviewed
Jul 6, 2026
ladinesa
reviewed
Jul 6, 2026
ladinesa
reviewed
Jul 6, 2026
ladinesa
reviewed
Jul 6, 2026
ladinesa
reviewed
Jul 6, 2026
ladinesa
reviewed
Jul 6, 2026
ladinesa
reviewed
Jul 6, 2026
ladinesa
reviewed
Jul 6, 2026
- Octopus: remove the dead `'._initial_system'` model_system annotation (immediately overwritten by the `get_systems` annotation on the same key), the writer-side `_initial_system` payload injection, and the unused `get_initial_system_for_mapping`; `get_systems` alone covers static calculations - Octopus: move `append` out of the geometry-step `try` block and log unreadable step files once after the loop; same for the coordinate-file candidates - Octopus: inline the single-caller `get_reference_energy` into `get_eigenvalues` - Crystal: attach the label regex to `CrystalOutputParser` instead of module scope - QE: pass `.atomic_structure.cell` as annotation argument to the XML `get_periodic_boundary_conditions` instead of resolving a `Path` inside the function - QE: key the PWSCF DOS payload cache by mainfile via class attributes; the previous `hasattr` guard leaked the payload of a previously parsed mainfile into subsequent parses on the shared parser instance - QE: annotate `get_reference_energy` return type
ladinesa
reviewed
Jul 7, 2026
| _dos_cache_key: str | None = None | ||
| _cached_dos_payload: dict[str, Any] | None = None | ||
|
|
||
| def get_dos(self, source: dict[str, Any]) -> list[dict[str, Any]]: |
Collaborator
There was a problem hiding this comment.
Strictly, I would suggest to make a new mapping parser for dos. Or at the very least, we add the reading of the dos data in def to_dict. This way we do not have to worry about caching.
Collaborator
There was a problem hiding this comment.
Agreed that a dedicated parser (or folding the read into to_dict) is the cleaner end state — the current mainfile-keyed cache is correct but is compensating for structure. I'd take it together with the DOSCAR-parser refactor (#192) rather than grow this PR further; happy to open a dedicated issue if you prefer.
Collaborator
There was a problem hiding this comment.
Tracked in #190 (added a note there covering the dedicated DOS mapping parser / folding the read into to_dict to drop the mainfile-keyed cache).
- Move the XML->OUTCAR electronic-outputs backfill from `VASPParser` into `XMLArchiveWriter`, keeping the TODO(mapping-migration) marker - Extract the XC functional mapping table and resolution into `parsers/vasp/common.py`, shared by the OUTCAR and XML parsers; drop the `__import__` indirection - Wire OUTCAR `get_atoms` through a mapping annotation with `ions_per_type` and `species` as sources; it was previously dead code, so OUTCAR archives now gain `particle_states` (additive); handle numpy-array species entries when extracting labels - Pass sources via annotation arguments to the XML `get_atoms` (`modeling.atominfo.array`) and `get_positions` (positions and lattice vectors); drop the internal `Path` lookups and the now redundant `get_lattice_vectors` - Promote `as_list` to `parsers/utils/general.py` and reuse it in the XML parser - Move single-use module constants into their functions; nest the single-caller Fermi-energy helper inside `get_total_dos` - Remove the dead `get_ediff_unit` in both parsers
…parsers - h5md: guarantee parser handles close on conversion errors via try/finally in `write_to_archive` (per-call closing would tear down the shared h5 handle mid-parse) - Octopus: check file existence before `ase.read` in the coordinate and geometry-step loops instead of relying on the exception path - Wannier90: fix the `energy_fermi` quantity regex in `WInParser` (`\n\r` never matches), superseding the manual re-read fallback and the `get_reference_energy` accessor; keep writer-provided aux data (`k_path`, reference energies) in private parser attributes instead of injecting synthetic keys into parsed `data` - Wannier90: replace the `getattr` workflow guard with an explicit `None` check on the resolved DFT archive - Yambo: pass `valence_conduction`/`valence`/`conduction` as annotation arguments to `get_band_gaps`; drop `hasattr` guards and annotate the nested reference-energy helper - GPAW: use `self.data_object` (the initialized file-parser handle) throughout `GPWParser`; move single-use constants into `get_eigenvalues`; normalize ULM band-path payloads to plain dicts in `GPWFileParser._get_band_paths` so the transformer sheds its callable/dict guards
…er90 wiring - VASP XML: remove the cross-calculation fallback (_resolve_electronic_source); get_eigenvalues, get_band_gaps, and get_total_dos now operate strictly on the calculation subtree passed via the mapping annotation. Electronic payloads land only on the output whose calculation carries them. - Yambo: propagate highest_occupied onto each eigenvalues entry so the mapped band-structure sections receive the reference energy. - Wannier90: only include k_path in the band-structure payload when the win file provides one, avoiding a None entry. - Update VASP and Yambo tests to the per-calculation semantics.
Collaborator
|
@ladinesa pls let me know if you're fine with the open threads and close them if so. Ready to merge when you want. |
Integrates develop (Lobster parser #172/#187, VASP charge density #35, FHI-aims SCF convergence criteria #177, nmr schema pin #179) with the electronic-properties migration. Conflict resolutions: - schema_packages/vasp.py: union of develop's `ChargeDensity`/`VASPOutputs` and the migration's electronic-output classes (auto-merged, verified) - schema_packages/fhiaims.py: kept #177 SCF-criteria alongside migration - parsers/vasp/outcar_parser.py: kept both `CHGCARParser` and `get_xc_functionals` imports - tests/parsers/test_vasp_parser.py: kept both import sets and both `test_chgcar` and the electronic-output regression tests - pyproject.toml: kept nmr pin and the index-url change Validated: full suite 196 passed / 9 skipped; snapshot additivity check (pre-merge vs merged) shows no archive data loss.
The declarative `update_mode='append'` mapping on `VASPOutputs` silently dropped `charge_density` on released nomad-lab (1.4.2): the CHGCAR section was never appended, yielding 13 outputs instead of 14 and failing `test_chgcar`. It only worked with the unreleased deterministic-inheriting-sections nomad-FAIR fix, so the feature depended on an unreleased dependency. Populate `charge_density` directly in `OutcarArchiveWriter.write_to_archive` and drop the fragile CHGCAR mapping annotations. This is deterministic across mapping-parser versions (verified on nomad-lab 1.4.2, 1.4.3rc1, and the local dev build). A `TODO` tracks reverting to a declarative mapping once the parser gains a deterministic append mode.
ladinesa
reviewed
Jul 9, 2026
| chgcar_parser = CHGCARParser() | ||
| chgcar_parser.filepath = self.mainfile | ||
| chgcar_parser.convert(archive_data_parser) | ||
| if (chgcar_file := chgcar_parser.load_file()) is not None: |
Collaborator
There was a problem hiding this comment.
This is not the fix that is necessary. I am sure something changed in the schema that caused this to fail. Please investigate further.
ladinesa
reviewed
Jul 9, 2026
| # does not reliably create the section across mapping-parser versions, | ||
| # silently dropping charge density on released nomad-lab. This | ||
| # writer-side fallback is version-stable. | ||
| # TODO: revert to a declarative mapping once the mapping parser gains a |
Collaborator
There was a problem hiding this comment.
The TODO note was for merge case, where the def is not updated but the fix is already in place in nomad-file-parser. append should work nonetheless.
…pping" This reverts commit 0206f17.
The MPCDF GitLab registry was removed as index in ecbdc25 after CI hit errors resolving from it, but that was a transient registry downage. Public PyPI only carries stable nomad-lab (<=1.4.2), which lacks the mapping-parser fixes that the CHGCAR `update_mode='append'` relies on, so `test_chgcar` fails there. The MPCDF registry carries the nomad-lab pre-releases with those fixes (1.4.3.dev/rc). Restoring it as the primary index lets the declarative charge-density mapping work again, matching develop's resolution. Verified: installing via the MPCDF index resolves nomad-lab 1.4.3rc1 and the full VASP suite (incl. test_chgcar) passes with the declarative convert.
ndaelman-hu
approved these changes
Jul 9, 2026
ndaelman-hu
left a comment
ndaelman-hu
left a comment
Collaborator
There was a problem hiding this comment.
Extensively reviewed by @ladinesa and myself, using snapshots and copilot.
Collaborator
|
@ladinesa Found it — not an annotation regression. The CHGCAR Fix ( |
ndaelman-hu
added a commit
that referenced
this pull request
Jul 9, 2026
Catch up the branch to develop (#170 property-normalization migration) and reapply the nomad_file_parser import swap on top. cli.py and chgcar_parser.py remain on nomad.parsing.file_parser, matching the original migration scope. Update the abinit geometry-optimization convergence test to identify targets by threshold_type instead of section name: nomad_file_parser serializes the polymorphic convergence targets as the base WorkflowConvergenceTarget, and the EnergyConvergenceTarget/ForceConvergenceTarget subclasses add no quantities of their own, so no data is lost.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Executive Summary
This PR migrates fundamental system quantities and a subset of electronic properties (DOS, band structures, band gaps) for several simulation parsers in the
nomad-simulation-parserspackage to usenomad-simulations, implementing mapping annotations and ensuring compatibility with the downstream results normalizer.Main Objectives
archive.runpopulation to declarativeadd_mapping_annotationfor electronic output propertiesoutputs.electronic_*fieldsParsers Modified (18 total)
Core Electronic Property Migration
FHI-aims (parser.py)-- separated into another branch due to conflicts with Add KMesh support for FHI-aims parser #171Notes
(Attempted, not necessarily universal) Coverage:
Parser Logic Enhancements
get_dos(),get_band_structure(),get_band_gap()parsers/utils/general.py:link_output_to_final_representative_system()- robust output linkingCoverage by Parser:
FHI-aims: +86 lines-- separated into another branch due to conflicts with Add KMesh support for FHI-aims parser #171Migration Compliance
Adherence to Migration Rules
✅ No runschema dependencies in parsers or schema packages
✅ Pure nomad-simulations objects in
archive.data✅ Mapping annotations for population (not manual archive assignment)
✅ Legacy parity maintained where tested
Parser-Specific Notes
Breaking Changes
None - this is additive migration maintaining backward compatibility.
Testing Strategy