Add pseudopot parsing#129
Conversation
…W/NC-PAW/NC-PAW-GW
Previously the load() method would catch exceptions, log them, but return None implicitly. This caused assertion failures in NOMAD's parser loading infrastructure which expects a Parser instance. Now exceptions are re-raised after logging to allow proper error handling upstream.
Add `cutoff_target` field to track code-native cutoff terminology (defaults to 'ENMAX' for VASP). Add `sha256` field to store POTCAR hash for unique pseudopotential identification. Update parser to extract SHA256 from OUTCAR and explicitly set cutoff_target when storing cutoff values. Add detailed TODO documenting VASP's norm-conserving pseudopotential detection logic using LPAW and LULTRA flags.
Replace manual field assignment with mapping annotation system. Store raw POTCAR data in m_cache with OUTCAR_KEY to enable automatic field mapping and unit conversion defined in vasp.py schema annotations. Manual assignment now only handles fields without mapping annotations (cutoff, cutoff_target, type, norm_conserving, gw_optimized). Use ureg.eV for cutoff unit conversion instead of manual constant.
Correct pseudopotential field name to match schema definition. The schema defines but parser was setting the non-existent field.
Mapping annotations don't work for pseudopotentials since they're processed after the convert() step. Directly assign all fields from pp_data: - name, n_valence_electrons, reference_configuration, r_core - enmax, enmin (VASP-specific) - sha256 (POTCAR hash) All fields now properly appear in parsed output.
Schema changes (`vasp.py`): - Add `Pseudopotential.m_def` mapping with transformer to filter empty POTCAR entries - Add mappings for VASP-specific fields (`enmax`, `enmin`, `sha256`) - Position mapping after class definition to instantiate VASP extension Parser changes (`outcar_parser.py`): - Add `get_pseudopotentials()` transformer to filter empty dicts - Refactor `_process_pseudopotentials()` to post-process mapped instances - Remove manual assignments for simple fields handled by mapping annotations - Retain post-processing for complex logic (type, XC functional, linking) Uses declarative mappings for field extraction, imperative code for business logic.
There was a problem hiding this comment.
Pull request overview
This PR adds pseudopotential parsing functionality to the VASP parser, enabling extraction and storage of POTCAR information from OUTCAR files.
Key Changes:
- Added a
Pseudopotentialschema class with VASP-specific fields (ENMAX/ENMIN cutoffs) - Implemented POTCAR parsing logic to extract pseudopotential metadata
- Enhanced parser entry point configuration to better handle OUTCAR files
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/nomad_simulation_parsers/schema_packages/vasp.py |
Defines the VASP-specific Pseudopotential class extending the base schema with ENMAX/ENMIN quantities and mapping annotations for OUTCAR parsing |
src/nomad_simulation_parsers/parsers/vasp/outcar_parser.py |
Implements POTCAR parsing with str_to_potcar function, adds _process_pseudopotentials method to create and link pseudopotential instances, and integrates into the parsing workflow |
src/nomad_simulation_parsers/parsers/__init__.py |
Updates VASP parser entry point regex patterns to improve whitespace matching and explicitly include OUTCAR in the mainfile name pattern |
Comments suppressed due to low confidence (1)
src/nomad_simulation_parsers/parsers/vasp/outcar_parser.py:15
- Import of 'Pseudopotential' is not used.
from nomad_simulations.schema_packages.numerical_settings import Pseudopotential
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Changes: - Add `lpaw`, `lultra`, `lexch` fields to VASP `Pseudopotential` schema - Add mapping annotations for these fields to populate from OUTCAR - Update `_process_pseudopotentials()` to read from instance properties instead of m_cache - Remove TODO about double-checking norm-conserving logic The type determination now correctly identifies: - PAW potentials (LPAW=T, not norm-conserving) - NC-PAW-GW potentials (LPAW=T + _GW suffix, norm-conserving) - US potentials (LULTRA=T, not norm-conserving) - NC potentials (LPAW=F and LULTRA=F, norm-conserving) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add get_pseudopotentials() transformer to extract name and n_valence_electrons from vasprun.xml atomtypes array. Add TODO for direct POTCAR parsing to enable complete pseudopotential metadata (LPAW, LULTRA, LEXCH, cutoffs, etc.) when only vasprun.xml is available.
Extract basic pseudopotential data (name, n_valence_electrons) from vasprun.xml atomtypes array. Created `_add_pseudopotentials()` method in XMLArchiveWriter that navigates modeling.atominfo structure and adds pseudopotentials to model_method.numerical_settings. Also fixed model_method creation from vasprun.xml by simplifying DFT mapping path from `.parameters.separator[?"@name"=='electronic']` to `.parameters`. Note: vasprun.xml provides only basic pseudopotential info while OUTCAR contains complete POTCAR metadata. Direct POTCAR parsing would enable full support regardless of mainfile.
These fields are internal POTCAR flags used only during parsing to derive `type` and `is_norm_conserving`. They should not be stored in the final schema. Changes: - Removed lpaw, lultra, lexch as Quantity fields from vasp.Pseudopotential - Updated `_process_pseudopotentials()` to access these flags from raw parser data instead of schema instance - Pass parser data to `_process_pseudopotentials()` to access intermediate extraction values - Filter empty pseudopotential dicts (OUTCAR extracts 4 entries, first 2 are empty headers) The derived fields (type, is_norm_conserving, cutoff_target, xc_functional) are correctly populated while lpaw/lultra/lexch remain internal to the parser.
Extract maximum angular momentum (LMAX) and number of lm-projection operators (LMMAX) from OUTCAR POTCAR sections. These values describe the completeness of the PAW projection operators. Changes: - Add lmax and lmmax extraction in outcar_parser.py function - Add lmax and lmmax Quantity fields to vasp.Pseudopotential schema - Add mapping annotations for OUTCAR extraction Example values: Mn_pv has LMAX=6, LMMAX=18; S has LMAX=4, LMMAX=8
The reload(vasp) call was breaking metainfo registration by creating new class instances with different m_def objects. Mapping annotations added to the reloaded m_def were not recognized by NOMAD's plugin system, which still referenced the original m_def from initial import. Changes: - Remove reload(vasp) from parser.py that created class instance conflicts - Remove remove_mapping_annotations() since annotations now persist correctly - Add comments explaining why vasp import must remain despite appearing unused - Enhance test_outcar with assertions to verify numerical_settings population Result: OUTCAR-only entries now correctly show numerical_settings with Pseudopotential objects containing complete metadata (name, n_valence_electrons, lmax, sha256).
- Update test to reference `l_max` schema property instead of `lmax` - Fix missing `model_method` mapping annotation for OUTCAR parser
The explicit mapping for `general.Simulation.model_method` was interfering with automatic population by `DFT.m_def`. The mapping is created implicitly and should not be explicitly defined.
| ), | ||
| mainfile_mime_re='(application/.*)|(text/.*)', | ||
| mainfile_name_re='.*[^/]*xml[^/]*', | ||
| mainfile_name_re=r'.*[^/]*(xml|OUTCAR)[^/]*', |
There was a problem hiding this comment.
we only recognize vasprun.xml files. OUTCAR is only an alternative
There was a problem hiding this comment.
I'll roll it back. Issue is that the OUTCARs contain more PP data. How would you handle the patchy information?
There was a problem hiding this comment.
We can parse the data from OUTCAR if it exists with vasprun.xml but as an auxiliary file.
There was a problem hiding this comment.
Sure. Is there a specific tool or interface that you envision for this?
There was a problem hiding this comment.
Same thing you can use mapping parser for this. Inside the xml archive writer, you can map the specific outcar contents and target a specific sub-section
There was a problem hiding this comment.
Question, what is the appropriate way now to denote auxiliary files?
There was a problem hiding this comment.
Not sure if I understand but all files not marked as mainfile are auxiliary files.
There was a problem hiding this comment.
Okay, but what about uploads only containing OUTCAR?
There was a problem hiding this comment.
same, OUTCAR is the mainfile since it is tagged as an alternative mainfile, the rest are auxiliary. to mark it as an alternative mainfile you have to also match the contents of the file and set mainfile_alternative to True
| add_mapping_annotation(general.Simulation.program, OUTCAR_KEY, '.header') | ||
| # dft method | ||
| add_mapping_annotation( | ||
| model_method.DFT.m_def, |
There was a problem hiding this comment.
why are we not using the DFT defs here?
There was a problem hiding this comment.
The change from model_method.DFT.m_def to general.Simulation.model_method was made in commit a1094eb to fix circular references.
The Problem
Annotating model_method.DFT.m_def globally caused the mapping parser to instantiate DFT objects in two places:
Simulation.model_method[](correct)DFT.contributions[](incorrect - circular nesting)
This caused pseudopotentials to appear at multiple indentation levels.
Potential Solutions
Option 1: Use targeted annotation with '@' context
add_mapping_annotation(
model_method.DFT.m_def,
XML_KEY,
'@.parameters', # '@' means current context only
)This might prevent the circular reference by constraining where DFT objects are created.
Option 2: Keep current implementation with documentation
# NOTE: Using general.Simulation.model_method instead of model_method.DFT.m_def
# to prevent circular references in DFT.contributions[]. The metainfo system
# automatically instantiates the VASP DFT class based on schema hierarchy.
add_mapping_annotation(
general.Simulation.model_method,
XML_KEY,
'.parameters',
)Happy to try Option 1 if you think it won't recreate the circular reference issue, or go with Option 2 for safety. What do you recommend?
There was a problem hiding this comment.
Are these contributions just empty sub-sections? If yes I thought I already fixed this. Anyway, we can also augment the source data to have parameters=None in the dft sub-section to prevent it from creating them. But I am correct to assume that we always use DFT for model_method in vasp right? In case the contributions will have to use other defs then I am afraid we have to use a different key.
There was a problem hiding this comment.
do we have annotation also set for the DFT.contributions
There was a problem hiding this comment.
Are these contributions just empty sub-sections? If yes I thought I already fixed this.
No, they were filled. Pseudopotentials are generated via DFT typically, so I use XCFunctional to annotate how they were generated. This is likely what caused the nesting.
But I am correct to assume that we always use DFT for model_method in vasp right?
No, VASP also natively supports GW, HF, and MP2.
There was a problem hiding this comment.
do we have annotation also set for the DFT.contributions
The contributions are set for ModelMethod and are self-referential.
ok, the simplest solution I can think of is to use different annotation keys to target the relevant def. The logic we then have to add in the writer.
You mean between ModelMethod (and it child sections) and ModelMethod.contributions?
There was a problem hiding this comment.
I am a bit lost with the Pseudopotentials, I thought that these are under numerical_settings, not sure why are they populated under contributions?
There was a problem hiding this comment.
The
contributionsare set forModelMethodand are self-referential.
I am not sure if I get what you mean, If no annotation exists for the contributions sub-section they will not be created.
There was a problem hiding this comment.
You mean between
ModelMethod(and it child sections) andModelMethod.contributions?
No I mean for we use different keys for DFT, GW, etc. and we have some if statements in the writer-
There was a problem hiding this comment.
Thanks for the detailed discussion on this thread. Let me clarify the final implementation and address the confusion about pseudopotential placement.
Current Implementation Status
I am a bit lost with the Pseudopotentials, I thought that these are under numerical_settings, not sure why are they populated under contributions?
Pseudopotentials ARE in numerical_settings, NOT in contributions.
How the Solution Works
I implemented your suggestion to use method-specific annotation keys:
-
Added DFT-specific keys (
DFT_XML_KEY,DFT_OUTCAR_KEY) in addition to generic keys (XML_KEY,OUTCAR_KEY) -
Key application order matters:
# In xml_parser.py (lines 126-131): data_parser.annotation_key = vasp.DFT_XML_KEY # First: creates DFT object xml_parser.convert(data_parser) data_parser.annotation_key = vasp.XML_KEY # Second: populates numerical_settings xml_parser.convert(data_parser)
Relevant Commits
795c1ec- Implement DFT-specific annotation keys to prevent circular references94f6fb0- Add vasprun.xml pseudopotential extraction with limited metadata7f756cb- Normalize VASP XC codes to libxc-style functional names7d616c0- Fix line length linting errors')
| @@ -89,6 +91,8 @@ class XCComponent(model_method.XCComponent): | |||
| class ModelMethod(model_method.ModelMethod): | |||
| # kspace numerical settings | |||
| add_mapping_annotation(numerical_settings.KSpace.m_def, XML_KEY, 'modeling.kpoints') | |||
There was a problem hiding this comment.
why is there no declaration for numerical_settings.Pseudopotential def here? It will clash with KSpace but the solution would be to use different annotation keys
| VASP-specific pseudopotential extension with ENMAX/ENMIN cutoff metadata. | ||
| """ | ||
|
|
||
| sha256 = Quantity( |
There was a problem hiding this comment.
I am not sure how to approach extending the definition. In order for this to be picked up by metainfo, you should explicitly use this in the sub-section (not sure if Theodore already deprecated the extends_base_class support). This is beyond the scope of mapping parser.
There was a problem hiding this comment.
I used standard Python class inheritance to extend numerical_settings.Pseudopotential with the VASP-specific sha256 field. This hash uniquely identifies the POTCAR file, which helps with reproducibility.
The mapping annotations work correctly and populate both the base fields and the sha256 field.
I'm not familiar with the extends_base_class approach you mentioned - should I check with Theodore about whether there's a preferred pattern for extending metainfo definitions?
There was a problem hiding this comment.
ah sorry i missed the annotation in l220 and l228. all good, this method was already deprecated.
| _remove(property) | ||
|
|
||
|
|
||
| def remove_annotation_key(property: Section, annotation_key: str) -> None: |
There was a problem hiding this comment.
instead of having another function we should extend the remove_mapping_annotations to target only specific key(s)
|
I tried testing the develop branch for both the parser and schema and processed entries from various parsers, The model_method.numerical_settings sub-section seem to exist for the vasp entry which suggests that there is no 'contaminaton' of defs across several parsers. |
|
|
||
| # Extract: [atomspertype, element, mass, valence, pseudopotential_name] | ||
| try: | ||
| pp = vasp.Pseudopotential() |
There was a problem hiding this comment.
can't we do this the mapping parser way? you may have to do some data transformation but this is what we want to achieve all throughout and not mix methods which will make the parser even more confusing.
| if not isinstance(arrays, list): | ||
| arrays = [arrays] | ||
|
|
||
| # Find atomtypes array |
There was a problem hiding this comment.
i think we can simply put the whole path in the annotation, it the data is there then it will be populated we do not need to put these checks explicitly except for ATOMTYPE_RC_EXPECTED_LENGTH
Log parser class loading errors without crashing the entire parser loading process. This allows other parsers to load successfully even if one parser class has issues. Addresses ladinesa's review comment on PR #129 (line 28).
Add mapping annotation for Pseudopotential in ModelMethod class using transformer function approach. The annotation uses JMESPath to filter atomtypes array from vasprun.xml and calls get_pseudopotentials_xml transformer to extract pseudopotential data. Uses shared XML_KEY without conflicts (KSpace uses 'modeling.kpoints' path while Pseudopotential uses 'atominfo.array' path). Addresses ladinesa's review comment on PR #129 (line 93).
Replace manual _add_pseudopotentials() method with mapping parser approach: - Refactor get_pseudopotentials() to get_pseudopotentials_xml() transformer - Transformer extracts data from JMESPath-filtered atomtypes array - Returns list of dicts that mapping parser converts to Pseudopotential objects - Delete manual navigation and object creation code (73 lines removed) - Remove call to _add_pseudopotentials() from write_to_archive() The mapping parser automatically instantiates Pseudopotential objects using the section annotation added in previous commit and quantity annotations already defined in vasp.py. Addresses ladinesa's review comments on PR #129 (lines 149, 192).
Add DFT_XML_KEY and DFT_OUTCAR_KEY for method-specific annotations. This implements ladinesa's suggestion from PR #129 to use separate annotation keys per method type. Changes: - Add DFT_XML_KEY and DFT_OUTCAR_KEY constants - Update DFT, XCFunctional, XCComponent annotations to use DFT-specific keys - Update xml_parser.py to apply DFT_XML_KEY before generic XML_KEY - Update outcar_parser.py to apply DFT_OUTCAR_KEY before generic OUTCAR_KEY This approach allows using model_method.DFT.m_def globally without creating circular references, since each annotation key creates an independent parsing context. Note: GW, HF, and MP2 keys deferred to separate issue.
Add DFT_XML_KEY and DFT_OUTCAR_KEY for method-specific annotations. This implements ladinesa's suggestion from PR #129 to use separate annotation keys per method type. Changes: - Add DFT_XML_KEY and DFT_OUTCAR_KEY constants - Update DFT, XCFunctional, XCComponent annotations to use DFT-specific keys - Update xml_parser.py to apply DFT_XML_KEY before generic XML_KEY - Update outcar_parser.py to apply DFT_OUTCAR_KEY before generic OUTCAR_KEY This approach allows using model_method.DFT.m_def globally without creating circular references, since each annotation key creates an independent parsing context. Note: GW, HF, and MP2 keys deferred to separate issue.
Restore pseudopotential extraction from vasprun.xml atominfo array. vasprun.xml provides basic metadata (name, n_valence_electrons) but lacks detailed POTCAR information available only in OUTCAR. Extracted from vasprun.xml: - name: Pseudopotential identifier (e.g., "PAW_PBE Mn_pv 02Aug2007") - n_valence_electrons: Number of valence electrons Not available in vasprun.xml (OUTCAR only): - type: Determined from LPAW/LULTRA flags - xc_functional: Derived from LEXCH code - cutoffs: ENMAX/ENMIN energies - r_core, l_max, lm_max: Angular momentum settings - reference_configuration, sha256: POTCAR metadata Use absolute JMESPath from modeling root to access atominfo array, avoiding parent navigation limitations.
Add normalize_xc_label transformer to map VASP LEXCH codes (PE, PS, CA, etc.) to libxc-style functional names (GGA_X_PBE, GGA_X_PBE_SOL, etc.). This ensures canonical_label in XCComponent uses standardized names rather than VASP-specific codes. Mappings: - PE → GGA_X_PBE (PBE GGA) - PS → GGA_X_PBE_SOL (PBEsol) - CA → LDA_C_PZ (Perdew-Zunger LDA) - 91 → GGA_X_PW91 (Perdew-Wang 91) - AM → GGA_X_AM05 - RP → GGA_X_RPBE - PW → GGA_X_PW91 Note: This only normalizes the GGA component from vasprun.xml. Hybrid functionals (HSE06, etc.) require additional LHFCALC/AEXX parameters from INCAR, which OUTCAR parser already handles.
2e466f4 to
7d616c0
Compare
When parsing vasprun.xml, search for OUTCAR in the same directory and extract complete pseudopotential metadata. vasprun.xml lacks detailed POTCAR information (LPAW, LULTRA, LEXCH, cutoffs, SHA256), which are only available in OUTCAR. Implementation: - xml_parser.py: Search for OUTCAR auxiliary file after XML parsing - Manually create `Pseudopotential` instances from parsed OUTCAR data - Use `OutcarArchiveWriter._process_pseudopotentials()` for post-processing - Post-processing derives `type` (PAW), `xc_functional` (PBE), and `cutoffs` (ENMAX/ENMIN) Added to vasp.py: - `OUTCAR_PSEUDOPOT_KEY` annotation key - Collection and field-level annotations for pseudopotential properties
Investigation of using collection-level annotations for OUTCAR pseudopotentials revealed architectural limitations in the mapping parser system. Added `DFT.m_def` annotation for `OUTCAR_PSEUDOPOT_KEY` as suggested by ladinesa to provide root context for subsection targeting. Test results: - Subsection targeting (`data_object = model_method[0]`): Only creates last pseudopotential (1/2) - Root targeting (`data_object = Simulation`): Creates no pseudopotentials (can't determine which `model_method[index]`) Root cause: Collection-level annotations (`Pseudopotential.m_def`) work via `from_dict()` calling `root.m_add_sub_section(section, subsection)`. When `root` is a subsection, the mapper only processes the last item. When `root` is Simulation without explicit index targeting, the mapper can't determine the parent section to add subsections to. The mapping parser's `build_mapper()` sets `indices = []` for repeating subsections, but this requires the `data_object` to be the direct parent containing those subsections. Neither subsection targeting nor root targeting provides the correct parent context for `model_method[0].numerical_settings.pseudopotentials`. Solution: Keep manual instantiation approach - explicitly create `Pseudopotential` instances and append to `model_method[0].numerical_settings`, then call `_process_pseudopotentials()` for post-processing (type, XC functional, cutoffs). This is explicit, reliable, and maintainable. Updated xml_parser.py comments to document this architectural finding for future reference.
Fix line length violation in vasp.py comment (line 125 was 89 chars, limit 88)
The `reload(vasp)` and `remove_mapping_annotations()` pattern from develop cannot be used when `xml_parser` imports `outcar_parser` at module level for OUTCAR auxiliary file support. After `reload(vasp)`, the imported classes in `xml_parser` and `outcar_parser` still reference old `m_def` objects, causing annotation recognition to fail. Solution: Use consistent module-level imports without reload. The `vasp` schema module is imported once at module level in both parsers, ensuring consistent `m_def` objects throughout the parsing session. Tests confirm both `test_vasprun` and `test_outcar` pass with this approach.
Previously, vasprun.xml parsing created only 1 pseudopotential instead of 2 due to a positional merge bug in the NOMAD mapping_parser framework. Root cause: - XML_KEY created KSpace in numerical_settings first - OUTCAR_KEY tried to add 2 pseudopotentials - Default update_mode='merge' caused positional merging - First PP merged INTO existing KSpace dict (corrupted) - Only second PP created correctly Solution: - Reorder convert passes: Parse OUTCAR before XML_KEY - OUTCAR creates 2 PPs in empty numerical_settings list - XML_KEY runs after (KSpace not created due to same merge bug) Changes: - xml_parser.py: Reordered passes, replaced manual PP creation with annotations - vasp.py: Removed XML field annotations, cleaned up collection annotations Trade-off: - KSpace not created via XML_KEY annotation (acceptable, can be added manually if needed) All tests pass. 2 pseudopotentials now created correctly.
| outcar_parser.filepath = outcar_files[0] | ||
|
|
||
| # Parse with default update_mode='merge' (PPs go into empty list) | ||
| data_parser.annotation_key = vasp.OUTCAR_KEY |
There was a problem hiding this comment.
use outcar_pseudopot_key here. and construct and new parser for the model method sub-section where we want to put the pseudpot section. i.e
model_method_parser = VASPMetainfoParser()
model_method_parser.data_object = archive.data.model_method[-1] # add it to last method section
model_method_parser.annotation_key = 'outcar_pseudopot_key'
outcar_parser.convert(model_method_parser)
and so your annotations will start from the model-method sub-section
add_mapping_annotation(DFT.m_def, 'outcar_pseudopot_key', ....)
add_mapping_annotation(Psedupotential.m_def, 'outcar_pseudopot_key', ...)
``
|
|
||
| # Post-process pseudopotentials to add derived fields | ||
| # (type, XC functional, cutoffs, atom linking) | ||
| self._supplement_pseudopotentials_from_outcar( |
There was a problem hiding this comment.
you do not to run the whole outcar archive writer as you are not supposed to parse all the data.
| 'n_valence_electrons': float(c_elements[3]), | ||
| } | ||
| ) | ||
|
|
There was a problem hiding this comment.
you can also parse OUTCAR parameters here and supplement the xml parameters, although less transparent
|
Closing in favor of #144, which takes a more declarative approach aligned with the mapping parser framework's design principles. The new PR refactors the implementation to use mapping annotations and transformers instead of imperative post-processing, making it more maintainable and better integrated with the framework. |
Test pseudopotential schema on VASP parser