-
Notifications
You must be signed in to change notification settings - Fork 79
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
Support parsing QE 6.8 outputs #717
Conversation
Thanks @qiaojunfeng . So does that mean that adding this won't help for people running v6.8 and parsing will fail? If that is the case, I think we should add the schema that works with v6.8 and then add this schema once the fix is released in a patch version of QE. Otherwise it seems to make little sense to me. EDIT: just gave this a try myself to see the behavior. When running
I have posted in the linked Gitlab PR to ask whether the fix will be released in a patch version of v6.8. If that is the case, then we can merge this since once people update to the latest patch of v6.8 all will work fine, and for lower patch versions there is just the error message but which can be ignored safely. |
Thanks @qiaojunfeng, and @sphuber for the review! I think since QE will not release a patch version, this will always be in 6.8. There are 2 options:
@sphuber do you agree? |
I think we should get this in somehow just to make 6.8 runnable, but I am not sure of keeping the error. I can already see the questions being asked over and over again by users, and the errors will also get stored in the database taking up space, even though we know it is a dud. I would try to see if we can ignore this particular error because ultimately it is not really important and doesn't matter whatsoever for the user. |
So there is no XML validation error in logger.
OK, so I add some code to remove that problematic XML element for QE 6.8 output, so the |
In QE develop branch, the XML timing info error is fixed, however the version is still 6.8, this might lead to a TypeError since in that case the `partial_pwscf` is `None`.
Thanks @qiaojunfeng! I did a little test run for a magnetic system, and unfortunately it seems that the output format for the magnetic moments has changed, resulting in the following error:
This is the format for the magnetic moments in v6.8:
Compare that to the following example from v6.7:
This messes up the parsing: aiida-quantumespresso/aiida_quantumespresso/parsers/parse_raw/pw.py Lines 641 to 643 in c49def7
Going to check if the magnetic moments are in the XML now. If not, I have a fix somewhere for this when we were doing some test runs on the QE develop branch, will dig that up. |
No luck, I'm afraid. I can't seem to find the magnetic moments of each site in the XML. Here's the lines I added to fix this after: aiida-quantumespresso/aiida_quantumespresso/parsers/parse_raw/pw.py Lines 641 to 643 in c49def7
Will try to do this in my branch on thanos and see if the magnetic moments are parsed properly. |
Thanks @mbercx!
Can you ping him please? |
@mbercx I guess what you are describing is more or less a separate problem is it not? Shall we already merge this, and then I let you open a PR to fix the parsing of magnetic moments from the stdout? |
@sphuber sure, that'd work as well. 👍 Just figured it would fit in the topic of this PR, but I wouldn't want to introduce scope creep for @qiaojunfeng! @giovannipizzi Sure! Will open a new issue an ping him there. |
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.
Thanks a lot @qiaojunfeng
…am#717) QE v6.8 has been released but the XML output it produces follows a new schema. The schema is added such that the `PwParser` can parse the XML content. Note that due to a bug in the output writing code, the output actually does not respect the schema and so parsing will fail with an error. The bug has been fixed in this commit: https://gitlab.com/QEF/q-e/-/merge_requests/1531 However, the fix is not expected to be released in a patch version but will rather ship with the next major release. Therefore, we add an exception in our own parser to ignore the duplicated value in the XML output if it is produced by QE v6.8. In this way, the database will not be spammed with unnecessary warning logs that have no further influence on the quality of the results.
Hi,
This PR adds the XML schema and a test for QE 6.8 output.
Note due to a bug in QE 6.8 output XML, this PR works only after applying this (freshly merged 😅) patch https://gitlab.com/QEF/q-e/-/merge_requests/1531 to QE.
Fix #713