-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
131 fhi aims unclear extraction of species #138
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments and questions. Very nice.
By the way, the pytest is failing. Can you check what is going on? Also, please check pycodestyle for utils.
@@ -88,6 +88,30 @@ class x_fhi_aims_section_controlIn_basis_func(MSection): | |||
- | |||
''') | |||
|
|||
x_fhi_aims_controlIn_basis_func_gauss_l = Quantity( | |||
type=np.dtype(np.int32), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use type=np.int32
here and in other quantities.
sec_basis_func.x_fhi_aims_controlInOut_basis_func_gauss_l = val[i][0] | ||
alpha = np.array(val[i][1]) / ureg.angstrom ** 2 | ||
sec_basis_func.x_fhi_aims_controlInOut_basis_func_primitive_gauss_alpha = alpha | ||
# From legacy versions we know that 'free-atom' or 'free-ion' are connected to 'occ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't comment into these code-specific changes too much, as the quantities do not have any description (and you might not want to add anything).
But if these quantities are being parsed in other places, then I'll simply delete this comment and possibly add it into the description of the method parse_atom_type
itself.
@@ -84,6 +89,54 @@ def get_files(pattern: str, filepath: str, stripname: str = '', deep: bool = Tru | |||
return filenames | |||
|
|||
|
|||
def hash_section( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let's see if I understand this method (feel free to correct me if not).
You want to pass the basis_set section (like above for FHIaims) to a hash inside the basis_set section itself. Not sure how this will help for comparison across entries, I guess depending on the settings the hash will be assigned so that basis_sets with the same setting have the same hash?
I like it, but in its current status, is this a real utility function? I mean, are you planning on using this for other parsers? I guess so, just wanted you to confirm this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think that a wrapper for a util.hash for msections is too specific for a util function.
elif key in ['cut_pot', 'radial_base']: | ||
setattr(sec_basis_set, 'x_fhi_aims_controlIn_%s' % key, np.array( | ||
val[0], dtype=float)) | ||
else: | ||
try: | ||
setattr(sec_basis_set, 'x_fhi_aims_controlIn_%s' % key, val[0]) | ||
setattr(sec_basis_set, 'x_fhi_aims_controlIn_%s' % key, v[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sec_basis_set.m_set(sec_basis_set.m_get_quantity_definition(f'x_fhi_aims_controlIn_{key}'), v[0])
try: | ||
evaluation_settings = zip(sections, subsections) | ||
except Exception: # TODO: specify exception | ||
raise ValueError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log error, no raise
`sections`: sections to be hashed together | ||
`subsections`: list of bools, indicating whether to include subsections. Must be of same length as basis_settings. | ||
`mode`: str, either `include` or `exclude` (default) | ||
`quantities`: list of str, quantities to be included or excluded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it not be better if instead of mode and quantities, you have (in)exclude: list of quantities to (in)exclude
`mode`: str, either `include` or `exclude` (default) | ||
`quantities`: list of str, quantities to be included or excluded | ||
''' | ||
sections = [sections] if isinstance(sections, MSection) else sections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sections = [sections] if not isinstance(section, list) else sections
This is an intermediate implementation. It removes
controlInOut
forcontrolIn
, which has been patched.It also introduces general functions for producing tier hashes.
The latter still have to be applied, but this requires an update of the tier references.
Atm, this was done in an external project, but I can make it run locally. I just need to merge this first.
Maybe we can already merge this and add the rest later? Depends on how much time I get to work on this.
Closes #131 and https://gitlab.mpcdf.mpg.de/nomad-lab/nomad-FAIR/-/issues/1576