Random parser improvements #239
Conversation
|
@ondracka I'll plan your this review in for tomorrow |
electronicparsers/fhiaims/parser.py
Outdated
| re_float = r'[-+]?\d+\.\d*(?:[Ee][-+]\d+)?' | ||
| re_float = r'[-+]?\d+\.*\d*(?:[Ee][-+]\d+)?' |
There was a problem hiding this comment.
This is to support cases like 124e-2?
If you consider the decimal optional, maybe use ? instead of *.
electronicparsers/fhiaims/parser.py
Outdated
| 'integer': None, | ||
| 'cubic': None, |
There was a problem hiding this comment.
Do you think we should add integer and cubic to the main schema? Any suggested names?
Note that in the new schema, you'll be able to extend the definition yourself.
electronicparsers/fhiaims/parser.py
Outdated
| 'cold': 'marzari-vanderbilt', | ||
| } | ||
|
|
||
| sec_smearing.kind = occupation_types_dict.get(val[0]) |
There was a problem hiding this comment.
Add a try block to capture IndexError
There was a problem hiding this comment.
And include val[1] too, pls
electronicparsers/fhiaims/parser.py
Outdated
| } | ||
|
|
||
| sec_smearing.kind = occupation_types_dict.get(val[0]) | ||
| # width is defined as unitless in the metainfo (should be Joules) |
There was a problem hiding this comment.
Agreed, let me add this as a TODO.
electronicparsers/fhiaims/parser.py
Outdated
| @@ -2224,6 +2242,10 @@ def _prep_elemental_tier(basis_settings: MSection) -> dict[str, Any]: | |||
| if hybrid_coeff is not None: | |||
| # is it necessary to check if xc is a hybrid type aside from hybrid_coeff | |||
| sec_method.x_fhi_aims_controlIn_hybrid_xc_coeff = hybrid_coeff | |||
There was a problem hiding this comment.
Other TODO for me:
The old schema has the alpha parameter (exact_exchange_mixing_factor) in the results, but now I see that it's missing this in run. data has it again, so the future parser should use that naming.
| kpoints_multiplicities.append(2) | ||
| sec_k_mesh.points = np.array(kpoints).astype(complex) | ||
| sec_k_mesh.multiplicities = kpoints_multiplicities | ||
| eigenvalues.kpoints_multiplicities = kpoints_multiplicities | ||
| else: |
There was a problem hiding this comment.
maybe a comment saying # fall back to gamma-point
There was a problem hiding this comment.
Maybe
# if no explicit k-points are written we have gamma-point only
There was a problem hiding this comment.
I have your request about the k-line density scheduled next.
tests/test_openmxparser.py
Outdated
There was a problem hiding this comment.
Shouldn't the name of test_C2N2 be test_C2H2 instead?
Few random parser fixes, related to one Nomad datamining ongoing minithesis, authored by @SimonKratochvil and I did some initial review locally. The OpenMX and QE patches should hopefully be fine. Please double check the FHIaims patches, there I'm far from being an expert. Especially the FHIaims src_threshold_energy patch seems to introduce some normalizing issues with basis set !?!
The FHIaims test failures should be preexisting.