Repository navigation
Exciting parser: initial subclass-based schema refactor - #253
VonaCecilia wants to merge 44 commits into
Conversation
ndaelman-hu
left a comment
ndaelman-hu
left a comment
There was a problem hiding this comment.
Thank you for the PR and catching the mapping mistakes. Overall quite good, though, given that this was a small refactoring of the annotations' structure, so some minor slip-ups are to be expected.
There are quite a few comments, but a lot of them stem from the same principle kind of mistake. So really, there are just 2 actual big comments. I typically split them up, so it's easier for you to find and fix. Some comments are very suggestive and are marked as nitpicks or minor.
Summary. This PR applies 3 changes:
- refactor
schema_packages/exciting.pyfrom module-level mapping annotations into per-section subclasses - add smearing and spin-polarization annotations
- corrects units in
info_parser.py
The smearing annotation has a bug I believe, more about it below. This one is crucial to fix.
Suggestion for in the future
Change 1 should not change anything and change 2 is purely additive. Individually, or together, they are perfect cases to first run the "additive snapshot" against. I would thus have split this PR to leverage that test strategically.
Fortunately, i could still run this test by picking the right commits. The test surfaced a bug, which I suspect was introduced in change 1. Here is a full summary:
- Unit fixes (develop→e52eefb) — modifying, cleanly isolated: ~10 values on
delta_charge_abs, nothing else. Intended. - Smearing/spin annotations (e52eefb→a29adf0) — additive, clean: additions only, no modifications.
- Subclass refactor (a29adf0→tip) — expected to be identical, but it isn't: it drops the smearing name='Gaussian' and adds empty Smearing sections under
model_method.contributions. Root cause + the lookup-path change are inline.
Failing CI/CD after Update
The tests are indeed expected to fail when changing the units, because they were written based on a false understanding of the output. We typically attempt to keep our CI/CD green and therefore will fix the tests within the same PR. It should suffice to modify the numerical reference values.
| # add_mapping_annotation( | ||
| # general.Simulation.model_method, | ||
| # INPUT_XML_KEY, | ||
| # '.input.groundstate', | ||
| # m_def=model_method.DFT.m_def, | ||
| # ) | ||
|
|
||
| # add_mapping_annotation( | ||
| # model_method.DFT.m_def, | ||
| # INPUT_XML_KEY, | ||
| # '.input.groundstate', | ||
| # ) |
There was a problem hiding this comment.
I would remove these comments if their functionality is covered.
There was a problem hiding this comment.
This one should be verified after the fix in the smearing. In principle, this mapping was present in the version without subclasses, but now, by uncommenting this, the smearing is not populated properly. I left it here to be sure to include it and to get feedback about this behavior.
|
|
||
| class ModelMethod(model_method.ModelMethod): | ||
| add_mapping_annotation( | ||
| model_method.ModelMethod.numerical_settings, |
There was a problem hiding this comment.
Blocker (data loss). This changes the parser output: annotating the shared ModelMethod.numerical_settings (which contributions also inherit) creates an empty Smearing section at every contribution level and loses the smearing name (name='Gaussian').
The name is lost because Smearing.name's lookup (initialization.smearing_kind) is resolved relative to wherever the Smearing section is anchored:
- Before (direct
Smearing.m_defannotation): anchored at the root → readsinitialization.smearing_kind→Gaussian. - Now (via
m_def=onModelMethod.numerical_settings): anchored under themodel_methodnode, which is itself sourced frominitialization.xc_functional→ effectively looks forinitialization.xc_functional.initialization.smearing_kind→ nothing.
Annotate Smearing.m_def directly instead (as before).
There was a problem hiding this comment.
How the searing should be populated is not clear to me; we should look at this together. I tried several things, and it often appears at the data level as in the example
"data": {
"m_def": "nomad_simulations.schema_packages.general.Simulation",
"name": "Gaussian",
"representative_system_index": 0,
"program": {
"name": "exciting",
"datetime": "2026-09-25T13:09:58.715776+00:00",
"version": "BORON9"
},
In this way, I am able to populate this:
"contributions": [
{
"numerical_settings": [
{
"m_def": "nomad_simulations.schema_packages.numerical_settings.Smearing",
"name": "Gaussian"
}
]
}
add_mapping_annotation(..., m_def=X) stored X in Section.more['mapper_m_def'] — unkeyed state on a definition shared by every parser. The mapping parser pops that slot for whichever parser builds its mapper first and discards that parser's own polymorphic annotations: importing the orca schema package was enough to strip KSpace/SelfConsistency from any FHI-aims parse in a full-plugin process (nomad parse, Oasis), with parse-order-dependent results. The slot is redundant: the helper already re-targets the keyed annotation onto the m_def subclass, which the polymorphic scan resolves under the parser's own key. Verified orca DLPNO output unchanged and FHI-aims KSpace restored regardless of import order.
…y (e.g. 'PBE', 'PBE0', 'B3LYP', 'SCAN+rVV10')
…understand if n_point should be the total one or the reduced one.
8098f8f to
0a9b54c
Compare
…rlization, k_mesh, smearing, spin_polarized
…basis set, atomic states, model system
…ntainer, model_system, smearing, kmesh, is_spin_polarized)
- map exciting smearing schemes onto the `Smearing.kind` enum: the Methfessel-Paxton orders to `'Methfessel-Paxton 1'` and `'Methfessel-Paxton 2'` (the order was returned as a one-element tuple), the extended linear tetrahedron method to `'Tetrahedra'` and the square-wave impulse to `'Square-wave'`; the new values need FAIRmat-NFDI/nomad-simulations `smearing-kind-tetrahedra-square-wave` - log a warning for an unknown smearing scheme or spin treatment instead of dropping it silently; type `get_is_spin_polarized` as `bool | None` - pass `initialization` to `get_k_mesh` through the mapping annotation instead of reading `self.data`, return no `KMesh` without a k-point grid, and drop the unmapped `n_reduced_points` key
- reader: one parametrized table for the smearing scheme and width, k-point grid, reduced k-point count, `rgkmax`, atom count and cell volume, plus the version hash, species mass and the charge distance in elementary charges - mapping: parametrized tables with ids for the XC type, smearing scheme, spin treatment and k-mesh transformers, including the absent, empty and zero rows and the warning on unknown values - integration: method settings on the fast-gate `C_gs` and `PbI_hybrids` fixtures; add the missing `assert` to three SCF checks of `C_minimal` and state the expected atom position as a literal - mark `test_model_system_normalization` as `pipeline` - extend the exciting test matrix
Purpose
This PR is not intended to deliver a conclusive version of the exciting parser.
The model_system, model_method, and program sections have been analyzed and compared with the legacy parser. The missing quantities have been categorized into three groups:
This PR addresses only the first group.
Scope
Included
Added subclasses to the exciting schema.
Added annotations for the following quantities:
Added integration, mapping, and pipeline tests for the new quantities annotated
Fixed a few bugs
Out of scope
Dependencies/merge order
Solves [#254]
Testing / Validation