126 apw missing energy parameters #139
Conversation
…xciting parsers + TODO: ascertain meaning of `matchingOrder` in `lo`
ndaelman-hu
added
bug
| key_vals_converted: dict[str, Any] = {} | ||
| for key_val in key_vals: | ||
| if len(key_val) != 2: | ||
| raise ValueError(f'Invalid key-value pair: {key_val}') |
There was a problem hiding this comment.
replace raise with a warning.
There was a problem hiding this comment.
This means that the basis_set output will be complete bull. When should an error be fatal?
There was a problem hiding this comment.
but we do not want to terminate parsing due to this. simply do not write to archive if deemed incorrect.
| @@ -283,3 +286,136 @@ def extract_polarization_outputs(): | |||
| workflow.m_add_sub_section(ParticleHoleExcitations.tasks, task) | |||
|
|
|||
| xs_workflow_archive.workflow2 = workflow | |||
|
|
|||
|
|
|||
| class OrbitalAPWConstructor: | |||
| except AttributeError: | ||
| self.wavefunctions[key] = np.append(self.wavefunctions[key], converted[key]) | ||
| elif val != converted[key]: | ||
| raise ValueError(f'Wavefunction {key} does not match previous value') |
There was a problem hiding this comment.
avoid raising exceptions
There was a problem hiding this comment.
Ok, finished with my review.
I can only comment on the fact that OrbitalAPWConstructor is not very understandable right now. Do you have any examples at hand that you can share? (I think that's why Alvin asked you for testing on this).
Furthermore, I would be more verbose. Especially with names of functions, descriptions, names of variables, etc. You can also add comments to make it even more clear.
Besides these comments, do you mind checking the getattr / setattr thingy? Also, pycodestyle is failing.
| return dict(sorted({**settings, **kwargs}.items())) | ||
|
|
||
| def append_wavefunction(self, settings, **kwargs): | ||
| ''' |
There was a problem hiding this comment.
Can you please add descriptions here and below in other functions?
electronicparsers/utils/utils.py
Outdated
| if self.wavefunctions: | ||
| for key, val in self.wavefunctions.items(): | ||
| converted = self._convert(settings, **kwargs) | ||
| if getattr(OrbitalAPW, key, {}).get('shape'): |
There was a problem hiding this comment.
@ndaelman-hu @ladinesa is this correct? I remember Theodore saying that one can safely use getattr for MSections, but just wanted to make sure 🙂
There was a problem hiding this comment.
getattr is fine, just avoid setattr.
| # convert to NOMAD section | ||
| formatted_orbital = OrbitalAPW() | ||
| for term, val in orbital.items(): | ||
| setattr(formatted_orbital, term, val) |
There was a problem hiding this comment.
Coming back to my comment above on getattr (which again, it is just to confirm, so most probably you can use it) setattr should not be used and instead you should m_add_sub_section.
You have an example in nomad/normalizing/method.py:ExcitedStateMethod, at the end I was adding to simulation depending on the dict self._method_def (which is defined at the beginning of the MethodNormalizer().
There was a problem hiding this comment.
you can do formatted_orbital.m_set(formatted_orbital.m_get_quantity_defintion(tem), val)
| ['l_quantum_number', 'j_quantum_number', 'k_quantum_number'] | ||
| ) | ||
|
|
||
| def _convert(self, settings: dict[str, Any], **kwargs) -> dict[str, Any]: |
There was a problem hiding this comment.
I think this could actually live out of OrbitalAPWConstructor, but you decide. Furthermore, the naming of the function could be more verbose: _convert_to_dict.
| ''' | ||
| Return the stored orbitals as a sorted list of `OrbitalAPW` sections. | ||
| ''' | ||
| def _sort_func(orbital: dict[str, Any], term: str): |
There was a problem hiding this comment.
I am not sure what this function does.
Indeed, documentation is something that I still wanted to add. I am generating some figures to aid in the explanation: |
This is an intermediate solution. It adds the missing energy parameters, corrects the FP-APW parsing in exciting and already prepares for the next release.
It also provides some general functionalities for unrolling all APW orbitals.
TODO:
Closes #126