-
Notifications
You must be signed in to change notification settings - Fork 3
Add bandstructure support for qe #55
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
base: master
Are you sure you want to change the base?
Conversation
…on electronic parsers + Claude Sonnet 3.7 - Update main file discovery for all quantum espresso workflows
- Correct formatting issues
- Correct unit mapping - TODO: fix remaining bugs
…flowparsers/quantum_espresso_bands/parser.py` - Correct double assignment energy units
Pull Request Test Coverage Report for Build 15002848715Details
💛 - Coveralls |
Here are some notes on how this implementation should handle Fermi level searches based on discussions with Elena Molteni: Band structure calculations typically come in (2 or) 3 steps:
Match the calculation type based on step 3, and enrich the band structure with information from step 1. Regarding the level alignment, there is a cascade of sources to check:
|
I found an edge case in BANDS calculations (step 3), where none of the k-point information is displayed, reporting instead xk=( 0.00000, 0.00000, 1.00000 )
zone border point and non-symmorphic group
symmetry decomposition not available In these cases, step 2 still proceeds as regular. An alternative strategy would be to parse this file instead. |
@ladinesa Pls note that targeting BANDS rather than the Band Structure Calculation PWSCF, means that there is an empty entry being created for the latter. This entry will bear a FAILURE status. Any recommendations on how to handle it? Should I delete it from this parser? |
i am.noy sure why the other entry is empty. is this for the overall workflow ? |
Pls take a look at my comment above. Step 2 remains mostly vacant, as I don't process it. It is picked up by the |
I do not see any problem if it is parsed by another parser. The important thing is, you are able to link it with what is parsed by the band parser in a separate worklow entry. |
It is picked up by the mainfile scan of the Let me be clear: our new QE parser should likely target step 2 and perform the symmetry analysis itself. Ways to handle the empty entry:
|
well, it is an entry matched by the parser, it it has no output it does not matter, as the file contains no info. the other parser should not delete other entries. |
Largely, yes*. Then pls feel free to proceed with the review. *: I think being specific here matters, so that's why I'm reiterating. The file in step 2 contains less information than the one in step 3. There are 2 exceptions to that statement:
|
# Find PWSCF files and parse | ||
try: | ||
if (pwscf_file := self._find_files()) is not None: | ||
self.pwscf_parser.mainfile = pwscf_file |
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.
Thsi is already matched and parsed by the quantum espresso parser for PWSCF, you simply have to load the entry if you need to access info from the archive. This means that the BANDS parser should be executed after PWSCF. Set level to 2
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'm fine with this approach. I ended up going with a minimalistic parser here, since this is what the other QE workflow parsers did too.
I can apply this, if you're fine with deviating from the old approach.
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.
the other qe workflow parsers also do not match and parse pwscf files, look at the parsers init file. there is no deviation afaik, the only thing missing is connecting the entries to pwscf, we should do this in pwscf so we do the child archive generation in only one place
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.
Should your or I see to add this to the QE parser then?
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 also add it no problem, just do the bands parsing in this pr then.
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.
That would be appreciated, as you have a better view on what you want and how to handle child archives.
I will take a look during the review or after then.
Just to be clear: I remove all population of run
, except for bandstructures, correct? Or do I still copy over e.g. method
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.
yes just populate the band structure quantities, then the method and system, if you do not see them in the bands mainfile or if they are the same as in pwscf then do not parse them i simply link them with with the pwscf entry.
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.
method
and system
are not in the BANDS file. I'll remove any setting of them here then.
I will only link properties like Fermi level, necessary for the band structure.
) | ||
self._process_data() | ||
|
||
def _process_data(self): |
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 put this under parse function
|
||
if self.pwscf_parser.results: | ||
self._create_system_section(sec_run) | ||
self._create_method_section(sec_run) |
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.
perhaps also the method
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.
what do you mean here?
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 was referring to do the same as the previous comment i.e. simply linking the method section of the pwscf entry but if they are diferent then you parse it separately
- Rename BANDS text parser to `MainfileParser`
- Remove `_process_data` function header (not the code block) - TODO: incorporate `_extract_reference_energy`
@ladinesa I'm setting the missing band structure information from the QE parser (after the workflow setup). |
there is this parser function |
- Remove references to old NOMAD search
Migrated here from nomad-coe/electronic-parsers#279