-
Notifications
You must be signed in to change notification settings - Fork 7
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
148 handle changing symmetries in vasp #150
base: develop
Are you sure you want to change the base?
Conversation
@ladinesa Should I add the test referenced in the original issue? |
- Tie each `kpoints` to `eigenvalues` on a `n_calc` base
TODO: distinguish for the case of single-point calculations
…e the symmetry is not updated
5595e1f
to
3906e81
Compare
electronicparsers/vasp/parser.py
Outdated
self._kpoints_info = [] | ||
for kpts_occs in self.parser.get('kpoints'): | ||
if kpts_occs is not None: | ||
self._kpoints_info.append({}) |
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.
probably define info = {} then append this later. Is there no info about the iter #? I am just worried that the kpoint does not match the step. If there is none maybe put a check that len(kpoint_info) == len(calcs)
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've only seen 2 cases up to now: the k-points are only updated once, or they are updated at each ionic update.
I am storing the index now, and will consider using it for safety checks.
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.
Btw, in vasprun.xml
, it's rather the eigenvalues
that are out of sync with the no. steps.
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.
Pls, take a look at my comment below. Personally, I think we can go without any iteration index.
Or would you rather than a list
, prefer a dict
with a clearer index?
Instead, I'd rather have a way of moving k-points
under calculation
, but this isn't self-evident for OUTCAR
.
electronicparsers/vasp/parser.py
Outdated
@@ -1032,39 +1033,51 @@ def n_calculations(self): | |||
'listgenerated': 'Line-path', | |||
} | |||
|
|||
def _find_kpoints(self) -> list[str]: |
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 do not see this used anywhere except in kpoints_info. I suggest not defining 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.
Could move it into kpoints_info()
.
While I get the rationale behind reserving functions only for abstractions that are used more than once, I split it here for readability. This happens in other places of the VASP parser too. You let me know what you think.
Thanks for the fix. Please also add a test for this, maybe one outcar and one vasprun, you can also use this for the atom positions fix. |
…x for k-point extraction
TODO: correct the no. of extracted eigenvalues (and k-points) from vasprun.xml
Something to consider in the future, when we clean the VASP parser.
When it comes to eigenvalues, we mostly just need the no. irreducible k-points (at that step) to restructure the eigenvalues. |
Tackle changing symmetries (and k-point set) between ionic updates in newer VASP versions (about 6).
This solution packages
kpoints
into lists matching the no.calculations
. The cases of single-point calculations and the older format are also handled (forvasprun.xml
). No explicit information about the version is used.Closes #148