Skip to content
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

fix lda_plus_u_calculation in output_parameters #936

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

superstar54
Copy link
Member

fix #935

The dftU is in the dft section of the output.

@superstar54 superstar54 requested a review from mbercx May 21, 2023 04:28
@mbercx
Copy link
Member

mbercx commented May 21, 2023

Thanks @superstar54! What version of QE are you running here? Could you maybe also provide the corresponding output files (aiida.out, data-file-schema.xml) so I can set up a test for this?

@superstar54
Copy link
Member Author

PWSCF v.7.0.
Here are the files
dftU.zip

Thanks for testing!

@sphuber
Copy link
Contributor

sphuber commented May 21, 2023

Are we only supporting v7.0 and up now @mbercx ? And is the location of this keyword consistent for all other versions > v7.0?

@mbercx
Copy link
Member

mbercx commented May 21, 2023

Are we only supporting v7.0 and up now @mbercx ? And is the location of this keyword consistent for all other versions > v7.0?

Both good questions. Indeed, according to our compatibility policy

Starting from aiida-quantumespresso==4.0, the last three minor versions of Quantum ESPRESSO are supported. Older versions are supported up to a maximum of two years.

We should support v7.0-v7.2. As for the hubbard key word, I'm assuming that things might have changed with the new hubbard inputs in v7.1. However, perhaps we should let someone answer that has actually been running pw.x calculations with Hubbard corrections, @bastonero?

@bastonero
Copy link
Collaborator

Indeed, from v7.1 all the Hubbard related keywords in SYSTEM are dropped. Only the HUBBARD card should be specified, which is done via the new HubbardStructureData, and not directly from parameters of the builder.

Nevertheless, I know that QE did not really change internally the logic, and many parameters are still the same.

Conclusion: it might be that the xml still has these keywords (😅). I will check the xml of a recent calculation of mine and update you

@bastonero
Copy link
Collaborator

Here you have an example of dataschema using the latest (v7.2) QE version. Indeed, the output_parameters has incorrectly lda_plus_u_correction: False in my PwCalculation node.

Shall we also think of dropping the VERY old convention of lda_plus_u naming? A boolean hubbard_correction would do the job.

data.tar.gz

@mbercx
Copy link
Member

mbercx commented May 22, 2023

Here you have an example of dataschema using the latest (v7.2) QE version. Indeed, the output_parameters has incorrectly lda_plus_u_correction: False in my PwCalculation node.

Thanks!

Shall we also think of dropping the VERY old convention of lda_plus_u naming? A boolean hubbard_correction would do the job.

Yeah, the output_parameters could do with a good scrubbing. This will be part of the work that @sphuber has been doing on moving all possible parsing to the XML file. @sphuber perhaps you can open a PR with this work so we can start collaborating on it there? I remember I was still supposed to give you some files for additional testing, but forgot which ones. 😅

@sphuber
Copy link
Contributor

sphuber commented May 23, 2023

#940

Let's pull that over the line first indeed, before optimizing other changes in the output_parameters output. @mbercx is there anything we still want to release in the current major version? Or can we start merging stuff that can be considered breaking and that we will release with v5.0?

What about the ACWF stuff? Is that complete?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lda_plus_u_calculation in output_parameters is wrong
4 participants