Add scf annotation#181
Open
ladinesa wants to merge 13 commits into
Open
Conversation
6 tasks
ndaelman-hu
requested changes
Jun 2, 2026
ndaelman-hu
left a comment
ndaelman-hu
left a comment
Collaborator
There was a problem hiding this comment.
I'm sorry, but this implementation breaks the tests for me locally.
Moreover, parsing the Si_geomopt/out.out test file, only generates a single criterion (energy), whereas it should extract 3: energy, density, and eigenvalues.
Some fields (e.g. SelfConsistency.name or n_max_iterations) are either incorrectly filled, or left blank. This is minor compared to the criteria issue, though.
Collaborator
Author
This is not complete implementation, it is just to give you an idea how to improve it |
Add SelfConsistency annotation using multi-mapper pattern to extract convergence criteria (energy, density, eigenvalues) as separate instances alongside KSpace in DFT.numerical_settings. Changes: - Add convergence quantity parsers to out_parser.py - Implement get_scf_convergence_criteria() to transform parsed data - Add SelfConsistency.m_def annotation with function transformer - Add test case for multi-instance convergence criteria Depends on multi-mapper infrastructure in nomad-FAIR mapping_parser.py. Currently blocked by source path resolution issue ( returns None).
Changed function annotation from ['@'] to ['@'] (without leading dot) for proper jmespath current-node semantics. Test shows multi-mapper creates 3 SelfConsistency instances correctly and function returns proper convergence criteria data. Final debugging needed for index error in from_dict subsection removal.
- Fix line length in out_parser.py convergence_density regex - Remove f-string prefix from max_scf_iterations regex (no placeholders) - Fix line length in test_fhiaims_parser.py list comprehension - Remove unused imports from fhiaims.py schema (numpy, Quantity) - Sort imports in fhiaims.py schema
The annotation must be at module level (not inside DFT class) for multi-mapper detection to work correctly. The mapper needs to scan all inheriting sections to find multiple annotated subclasses.
Multi-mapper does not support transformers returning lists. Split `get_scf_convergence_criteria()` into 3 separate methods and manually create `SelfConsistency` instances in `write_to_archive()`. - `get_scf_energy_criterion()` - energy convergence threshold - `get_scf_density_criterion()` - density convergence threshold - `get_scf_eigenvalues_criterion()` - eigenvalues convergence threshold Result: `DFT.numerical_settings` now correctly contains 1 `KSpace` + 3 `SelfConsistency` instances.
All SCF convergence thresholds (energy, density, eigenvalues) now return plain floats in their printed units, avoiding mixed Pint Quantity/float handling.
Address PR review feedback about semantic ambiguity in convergence criteria identification. Previously, energy and eigenvalues criteria were both stored with 'eV' units and distinguished only by threshold magnitude (1e-6 vs 1e-3), which is fragile and error-prone. Changes: - Add explicit `name` field to each criterion type in parser helper functions: - 'total_energy_change' for energy convergence - 'charge_density_change' for density convergence - 'sum_eigenvalues_change' for eigenvalues convergence - Implement post-instantiation name assignment to work around `SelfConsistency.__init__` setting `self.name = self.m_def.name` - Update test to use name-based identification instead of threshold magnitude - Remove outdated comment from schema package This makes criteria semantically identifiable independent of threshold values, improving robustness and maintainability.
Address reviewer feedback to leverage annotation framework's unit handling instead of manually hardcoding unit strings in transformer functions. Changes: - Add `unit='eV'` parameter to text parser Quantity definitions for `convergence_energy` and `convergence_eigenvalues` in out_parser.py - Update transformer functions to extract magnitude and unit from pint Quantities using `format(units, '~P')` for short symbol notation - Handle both pint Quantity (with units) and plain float values in transformer functions for backward compatibility - Fix double-unit issue in `EnergyConvergenceTarget.threshold` assignment by checking if value already has units before multiplying This approach works with the current split-field schema pattern (`threshold_change` + `threshold_change_unit`) without requiring schema migration, as the transformers extract magnitude and unit string from the pint Quantity provided by the text parser. All tests pass.
Update FHI-aims parser to leverage the new SelfConsistency schema with flexible_unit=True. Eliminates inelegant string formatting workaround. Changes: - Remove magnitude/unit extraction logic from transformer functions - Pass pint Quantities directly to `threshold_change` field - Remove `threshold_change_unit` assignments (field removed from schema) - Update test assertions to check pint Quantity fields: - `threshold_change.magnitude` for numeric value - `threshold_change.units` for unit (electron_volt) - Density remains dimensionless (plain float) Benefits: - Cleaner code: eliminated ~15 lines of string manipulation - Type-safe: pint validates units automatically - Consistent: matches WorkflowConvergenceTarget pattern All tests pass.
ladinesa
force-pushed
the
feature/fhiaims-scf-convergence-alvin
branch
from
7bcc094 to
9d47584
Compare
Collaborator
Author
|
@ndaelman-hu @EBB2675 @JFRudzinski my initial idea to simplify the fhiaims parsing of scf conv quantites. Feel free to improve on it further. |
Coverage Report for CI Build 28442049444Coverage decreased (-0.02%) to 76.2%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Collaborator
Thx! I'll try parsing again with this to see whether it extracts all criteria. |
Collaborator
Author
|
@ndaelman-hu should we simply merge this? |
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.
Remove the manual creation of scf numerical setting section.