Refactor VASP pseudopotential parsing to use declarative mapping annotations#144
Open
ndaelman-hu wants to merge 20 commits into
Open
Refactor VASP pseudopotential parsing to use declarative mapping annotations#144ndaelman-hu wants to merge 20 commits into
ndaelman-hu wants to merge 20 commits into
Conversation
Refactor pseudopotential parsing to follow mapping parser framework idiomatically: **Schema (vasp.py)** - Add `Pseudopotential` class extending `numerical_settings.Pseudopotential` - Add `sha256` field for POTCAR file identification - Add collection annotations creating instances from transformer output - Add field annotations mapping dict keys to schema fields - Support both OUTCAR (complete metadata) and XML (limited metadata) **OUTCAR Transformer (outcar_parser.py)** - Add quantity definition to parse POTCAR headers from OUTCAR - Implement `get_pseudopotentials()` transformer performing ALL derivations: - Extract basic metadata (name, valence, r_core, l_max, sha256) - Derive `type` from LPAW/LULTRA flags and name patterns - Derive `is_norm_conserving` and `is_gw_optimized` from type - Derive XC functional from LEXCH parameter using VASP mapping - Derive cutoffs from ENMAX/ENMIN - Return plain dicts (not Section objects) following idiomatic pattern - No post-processing needed - all logic in transformer **XML Transformer (xml_parser.py)** - Implement `get_pseudopotentials_xml()` transformer - Extract limited metadata from vasprun.xml atomtypes array - Return name and valence electrons only (XML lacks detailed PP data) - Designed for multi-pass supplementing if OUTCAR unavailable **Key Improvements** - Declarative: Annotations control behavior, not imperative code - Single responsibility: Each transformer does ONE thing completely - No post-processing: All derivations happen during mapping - Reusable: Transformers can be used across different parsers - Maintainable: Logic centralized in transformers, not scattered Follows patterns from mapping_parser.py documentation added in docs/add-mapping-parser-pydocs.
Implements complete pseudopotential metadata extraction from VASP OUTCAR and vasprun.xml files using the mapping parser framework idiomatically. The parser now recognizes OUTCAR files at the parser level by overriding `is_mainfile()` to validate VASP content signatures, allowing direct parsing without modifying entry point regex. Schema annotations in `vasp.py` support field-level enrichment where XML creates basic instances with name and n_valence_electrons, while OUTCAR enriches with complete metadata including type, xc_functional, cutoffs, and all other fields. The framework automatically merges by position during multi-pass parsing. The OUTCAR transformer extracts POTCAR headers via regex and derives all metadata idiomatically: PP type from LPAW/LULTRA flags, XC functional keys from LEXCH parameter mapping to LibXC aliases, and cutoffs from ENMAX/ENMIN values. The XML transformer provides limited metadata extraction from atomtypes arrays in vasprun.xml. Nested subsections are handled by returning dicts that the framework converts: XC functional dicts with `functional_key` get expanded to LibXC components during normalization, and cutoff lists create `PPCutoff` instances with proper unit conversion from eV to joules. All fields now populate correctly including sha256 hashes for POTCAR library matching. OUTCAR parsing works completely while vasprun.xml parsing is preserved without regression.
- Corrected XML array index in `get_pseudopotentials_xml()` from `c_elements[2]` (atomic mass) to `c_elements[4]` (pseudopotential name), now correctly extracting names like "PAW_PBE Mn_pv 02Aug2007" instead of mass values - Relocated pseudopotential collection annotations from standalone `Pseudopotential` class to `ModelMethod` class to ensure pseudopotentials are properly added to `model_method.numerical_settings` - Set `mainfile_alternative=False` for VASP XML parser entry point Pseudopotentials now correctly appear in archive with name and `n_valence_electrons` from vasprun.xml files.
Modified `XMLArchiveWriter` to automatically detect and parse OUTCAR files in the same directory as vasprun.xml, using a third parsing pass with `OUTCAR_KEY` to extend pseudopotential metadata. The mapping framework merges data by list position, allowing OUTCAR to supplement the basic XML metadata (name, n_valence_electrons) with detailed fields (type, reference_configuration, cutoffs, xc_functional, r_core, l_max, lm_max, is_norm_conserving, is_gw_optimized). This multi-pass parsing approach enables vasprun.xml to provide the basic structure while OUTCAR adds comprehensive pseudopotential details when available, gracefully degrading to XML-only data when OUTCAR is absent.
Added explicit `update_mode='merge'` parameter to the `outcar_parser.convert()` call in `XMLArchiveWriter` to document the merging strategy. While 'merge' is the default, being explicit improves code clarity and aligns with patterns used in other parsers (octopus, exciting, abinit). The 'merge' mode aligns pseudopotentials by index position (source[0]→target[0]), recursively merging dict keys. This allows OUTCAR to extend XML pseudopotentials with detailed metadata (type, cutoffs, xc_functional) while preserving the XML structure.
Changed relative paths like to absolute paths like in XML parsing queries. The mapping parser's XMLParser requires absolute paths from the document root element. This fixes DFT model_method creation from minimal vasprun.xml files.
Changed path from 'modeling.atominfo.array' to 'atominfo.array' in the get_pseudopotentials_xml transformer arguments. Transformer paths in collection annotations should be relative to the m_def root context (set to 'modeling' for XML parsing) without including the root prefix. This follows the pattern used in other XML transformers like get_eigenvalues which uses ['eigenvalues'] not ['modeling.eigenvalues'].
Closed
Fix line length violations (E501) and magic value warnings (PLR2004) by: - Breaking long docstrings and comments across multiple lines - Extracting array indices to named variables with explanatory comments
Pull Request Test Coverage Report for Build 23485291974Details
💛 - Coveralls |
ladinesa
reviewed
Feb 4, 2026
ladinesa
reviewed
Feb 4, 2026
|
|
||
|
|
||
| class VASPParser(MatchingParser): | ||
| def is_mainfile( |
Collaborator
There was a problem hiding this comment.
why is this necessary? is the mainfile_alternative insufficient?
ladinesa
reviewed
Feb 4, 2026
ladinesa
reviewed
Feb 4, 2026
| mainfile_mime_re='(application/.*)|(text/.*)', | ||
| mainfile_name_re='.*[^/]*xml[^/]*', | ||
| mainfile_alternative=True, | ||
| mainfile_alternative=False, |
ladinesa
reviewed
Feb 4, 2026
ladinesa
reviewed
Feb 4, 2026
| LOGGER = get_logger(__name__) | ||
|
|
||
|
|
||
| def approx(value, abs=0, rel=1e-6): |
Collaborator
There was a problem hiding this comment.
we move this to parser utils
ladinesa
reviewed
Feb 4, 2026
Changes: - Set `mainfile_alternative=True` in parser entry point to enable auxiliary file processing - Enhanced `is_mainfile()` documentation explaining why override is necessary for standalone OUTCAR files - Simplified `_find_outcar()` to use basename check with `iterdir()` instead of explicit candidate list - Made filename matching case-insensitive for both discovery methods This creates a comprehensive OUTCAR discovery strategy that handles both standalone OUTCAR parsing and automatic discovery when accompanying vasprun.xml files. Testing confirms both scenarios work correctly: - vasprun.xml + OUTCAR: Extracts 2 pseudopotentials with complete metadata (name, type, valence, XC, cutoffs) - Standalone OUTCAR: Extracts 2 pseudopotentials with complete metadata
Extract lambda into parse_potcar_section() for better readability. Maintains all existing functionality while improving maintainability. Testing confirms both scenarios work correctly: - vasprun.xml + OUTCAR: 2 PPs with complete metadata - Standalone OUTCAR: 2 PPs with complete metadata
Implements field-level transformer pattern for modular per-instance field derivations, addressing PR #144 feedback. Changes: - Refactored `get_pseudopotentials` to passthrough `get_raw_pseudopotentials` - Added field-level transformers: `to_float()`, `to_int()`, `derive_pp_type()`, `derive_is_norm_conserving()`, `derive_is_gw_optimized()` - Changed transformers from list-typed to scalar (per-instance) signatures - Moved collection annotation to after `Pseudopotential` class definition so custom VASP class with extra fields is instantiated (not base class) - Converted field annotations to relative paths (`.TITEL` not `@.pseudopotentials[*].TITEL`) - Added multi-path field transformer annotations: `('derive_pp_type', ['.LPAW', '.LULTRA', '.TITEL'])` - Moved `approx()` test helper to shared `tests/parsers/utils.py` Technical: Field transformers called once per instance with scalar values. Framework extracts multiple paths per instance and passes as positional args. Tests pass: Both vasprun.xml and standalone OUTCAR produce complete metadata.
Changes `parse_potcar_section()` to return `None` for header-only POTCAR
lines (instead of empty dict). Framework skips `None` values automatically,
eliminating need for `get_raw_pseudopotentials()` filtering transformer.
Changes:
- outcar_parser.py: Return `None` from `parse_potcar_section()` for
header-only lines, remove `get_raw_pseudopotentials()` transformer
- vasp.py: Change collection annotation from
`('get_raw_pseudopotentials', ['@.pseudopotentials'])` to direct path
`'@.pseudopotentials'`
Result: Cleaner code, filtering handled at source by text parser.
Refactors POTCAR section parsing to use idiomatic TextParser sub-parser pattern instead of manual regex extraction function. Changes: - outcar_parser.py: Replace `parse_potcar_section()` function with declarative `potcar_quantities` list defining field patterns - Use `sub_parser=TextParser(quantities=potcar_quantities)` instead of `str_operation=parse_potcar_section` - Update regex pattern to filter header-only POTCAR lines with positive lookahead: `(?=[\s\S]*?VRHFIN)` ensures VRHFIN present before capturing - Add `dtype=int` to LMAX and LMMAX quantities for automatic type conversion Benefits: - More maintainable: Declarative quantity definitions vs manual parsing - More idiomatic: Uses TextParser framework patterns - Cleaner: ~40 lines of manual regex code eliminated - Type-safe: Framework handles type conversions automatically All tests pass.
Eliminates ~90 line manual traversal function by using JMESPath directly in
collection annotation for vasprun.xml pseudopotential parsing.
Changes:
- vasp.py: Replace `('get_pseudopotentials_xml', ['atominfo.array'])` with
JMESPath expression: `atominfo.array[?@name=='atomtypes'] | [0].set.rc`
- vasp.py: Update XML field annotations to extract from c array structure:
`.c[4]` for name, `('to_float', ['.c[3]'])` for valence electrons
- xml_parser.py: Add `to_float()` field-level transformer
- xml_parser.py: Remove `get_pseudopotentials_xml()` function entirely
Benefits:
- Declarative: JMESPath expression clearly shows data navigation
- Concise: ~90 lines of manual Python code eliminated
- Maintainable: No custom traversal logic to maintain
- Framework-native: Uses JMESPath and field-level transformers only
All tests pass.
…ments Refactors schema package to follow idiomatic style from other parsers like exciting.py. Quantities and their annotations are now co-located for clarity. Changes: - Group each custom quantity (sha256, lpaw, lultra, lexch, enmax, enmin) with its annotation immediately following the definition - Simplify docstring from verbose paragraph to concise one-liner - Remove noise comments: "dft method", "kspace numerical settings", "atomic cell", "value is already defined" explanations - Keep TODOs (they provide useful context) - Reduce Pseudopotential class from ~120 lines to ~70 lines Style matches exciting.py and other parsers: minimal comments, co-located definitions and annotations, clean and scannable. All tests pass.
When vasprun.xml is the mainfile, OUTCAR now only supplements pseudopotential metadata (SHA256, LPAW, LULTRA, ENMAX, etc.) instead of parsing all data. This avoids redundant parsing since vasprun.xml already contains eigenvalues, energies, and forces. Changes: - Create minimal `outcar_supplement_parser` with only pseudopotential quantity - Fix POTCAR regex to require VRHFIN within matched content, not just ahead: `POTCAR:([\s\S]+?VRHFIN[\s\S]+?)(?=\s*POTCAR:|\s*local pseudopotential:|\Z)` This prevents matching header-only POTCAR lines that lack detailed metadata - Simplify `_find_outcar()` to use generator expression with `next()` - Update comments to reflect POTCAR-specific scope Fixes issue where 4 pseudopotentials were parsed instead of 2 due to regex matching both header-only lines and detailed sections.
ladinesa
reviewed
Feb 6, 2026
| Quantity('LEXCH', r'LEXCH\s*=\s*(\w+)'), | ||
| Quantity('ZVAL', r'POMASS\s*=\s*[\d\.]+;\s*ZVAL\s*=\s*([\d\.]+)'), | ||
| Quantity('RCORE', r'RCORE\s*=\s*([\d\.]+)'), | ||
| Quantity('ENMAX', r'ENMAX\s*=\s*([\d\.]+)'), |
Collaborator
There was a problem hiding this comment.
specify the types here already so you do not need the conversion functions like to_float
ladinesa
reviewed
Feb 6, 2026
| outcar_path = self._find_outcar() | ||
| if outcar_path and os.path.exists(outcar_path): | ||
| LOGGER.info( | ||
| f'Found OUTCAR at {outcar_path}, extending vasprun.xml pseudopotentials ' |
Collaborator
There was a problem hiding this comment.
avoid formatted string pass outcar_path as extra
ladinesa
reviewed
Feb 6, 2026
| quantities=[ | ||
| Quantity( | ||
| 'pseudopotentials', | ||
| r'POTCAR:([\s\S]+?VRHFIN[\s\S]+?)(?=\s*POTCAR:|\s*local pseudopotential:|\Z)', |
Collaborator
There was a problem hiding this comment.
maybe reuse the one defined in the outcar parser so we do not have then in two places
ladinesa
reviewed
Feb 6, 2026
| data_parser.close() | ||
| xml_parser.close() | ||
|
|
||
| def _find_outcar(self) -> str | None: |
Collaborator
There was a problem hiding this comment.
can't we use the search_file in utils instead? sorry just realized it now.
Collaborator
|
I am ok with the general implementation now, just some more minor suggestions for improvement. |
ndaelman-hu
force-pushed
the
refactor/idiomatic-vasp-pseudopotential-parsing
branch
from
a1eabf2 to
18eab9c
Compare
…-vasp-pseudopotential-parsing
…-vasp-pseudopotential-parsing
- Split long regex patterns across multiple lines - Add noqa comments for justified PLC0415 (imports inside function) - Remove unused import in test_vasp_parser.py - Fix import formatting All VASP parser tests still pass (2 tests).
This commit fixes the broken pseudopotential extraction from vasprun.xml files. ## Root cause The module-level annotation `Pseudopotential.m_def` with path `atominfo.array[...]` was not working because: 1. The JMESPath filter syntax was incorrect (`@name` should be `"@name"` with quotes) 2. More critically, the annotation lacked a parent context - the mapping parser didn't know these Pseudopotential instances should be added to `ModelMethod.numerical_settings` ## Solution Added a custom mapper function `get_pseudopotentials()` to `VasprunParser` that extracts pseudopotential data from the `modeling.atominfo` section and returns it in a format the mapping parser can process. Updated the schema annotation to use this function with proper context. ## Changes `xml_parser.py:78-84` - Added `get_pseudopotentials()` method to extract PP data from vasprun.xml atominfo section `vasp.py:360-367` - Fixed pseudopotential annotation to use custom mapper function - Changed path syntax from incorrect `@name` to correct `"@name"` - Added comment explaining why custom mapper is needed (XML structure mismatch) `test_vasp_parser.py:15-35` - Added assertions to verify pseudopotentials are extracted with required fields - Accounts for OUTCAR merge deduplication behavior ## Testing - Both `test_vasprun()` and `test_outcar()` pass - Pseudopotentials now extracted from vasprun.xml with `name` and `n_valence_electrons` populated - OUTCAR merge continues to work, supplementing with detailed metadata
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
3 participants
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.
Purpose
Refactor VASP pseudopotential extraction to align with the declarative nature of the mapping parser framework, replacing imperative post-processing with annotation-driven data transformation.
Supersedes: #144 (old pseudopotential parsing PR)
Scope
Included
update_mode='merge'documentation for multi-file parsing strategyOut of scope
Context & Links
This approach aligns with the mapping parser's declarative philosophy:
Key technical fixes:
modeling.parametersnot.parameters)atominfo.arraynotmodeling.atominfo.array)These bugs prevented proper data extraction and required significant debugging through the mapping parser internals.
Reviewer Notes
Critical path resolution rules (took extensive debugging to discover):
Section mapping paths (for DFT, ModelSystem, etc.): Use absolute paths from XML root
modeling.parameters.separator[...].parameters.separator[...]Transformer function_args paths: Use paths relative to m_def context
m_def='modeling':['atominfo.array']['modeling.atominfo.array']This was counter-intuitive and not well-documented in mapping_parser.py. I've submitted documentation improvements to nomad-FAIR in branch
docs/expand-mapping-parser.Trade-offs:
Status
Breaking Changes
Dependencies / Blockers
docs/expand-mapping-parserto improve mapping parser documentationTesting / Validation
test_vasprun,test_outcar)cc @ladinesa - This supersedes the previous pseudopotential parsing work and takes a more framework-aligned approach using declarative mapping annotations.