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

Add OpenGridCalculation for open_grid.x code #714

Merged
merged 9 commits into from
Feb 9, 2023

Conversation

qiaojunfeng
Copy link
Collaborator

@qiaojunfeng qiaojunfeng commented Aug 5, 2021

QE has an open_grid.x code to unfold an irreducible-Brillouin-zone calculation to a full kmesh, significantly reduces the computation if the following calculation requires a full kmesh. This PR adds a OpengridCalculation class to support the code.

To allow subsequent ProjwfcCalculation on top of the OpengridCalculation, I have to introduce a somewhat ugly input/output spec:

  1. a structure input which isn't used by the OpengridCalculation, but because ProjwfcCalculation implicitly requires the parent calculation having an input structure (so it can parse the atomic orbitals).
  2. I modified the ProjwfcParser to allow it to use output kpoints if no input kpoints in the parent calculation (projwfc should use the output kpoints which is the full kmesh).

@sphuber
Copy link
Contributor

sphuber commented Aug 6, 2021

Thanks @Yunfeng. Before I review this, a quick couple of questions:

  1. Where exactly is the computation time won? If you pass an IBZ kpoints to a calculation, will QE try to do the unfolding itself but in a less effective way compared to open_grid.x? Surely calculating a full mesh is more expensive compared to a reduced mesh?
  2. The issue you describe of the ProjwfcParser already cropped up a long time before and is documented in ProjwfcParser uses input nodes up to 3 links back, and prevents easy testing #299 . Ideally, we should really just fix this, and then you also don't have to work around this with weird specs etc.

@qiaojunfeng
Copy link
Collaborator Author

Thanks @sphuber,

Thanks @Yunfeng. Before I review this, a quick couple of questions:

  1. Where exactly is the computation time won? If you pass an IBZ kpoints to a calculation, will QE try to do the unfolding itself but in a less effective way compared to open_grid.x? Surely calculating a full mesh is more expensive compared to a reduced mesh?

In essence, the open_grid.x just read the wavefunctions of IBZ kpoints, apply some symmetry operations to generate wavefunction at the remaining kpoints. For pw.x, we can do a nscf calculation on full kmesh with restarting from IBZ calculation, but that requires diagonalization of the Kohn Sham equations, and orthogonalization of KS orbitals, this is much slower than reading and rewriting wavefunctions. This difference is something like 30 mins v.s. 3 mins.

  1. The issue you describe of the ProjwfcParser already cropped up a long time before and is documented in ProjwfcParser uses input nodes up to 3 links back, and prevents easy testing #299 . Ideally, we should really just fix this, and then you also don't have to work around this with weird specs etc.

Yes I agree we should fix the ProjwfcParser, the question is how to fix it:
the ProjwfcParser requires a kpoints, a structure, and a output_parameters containing number_of_spin_components,

nspin = parent_param.get_dict()['number_of_spin_components']

In this opengrid case, the ProjwfcParser should use the opengrid output kpoints, since it is different from its input kpoints, so we can add an optional input kpoints to ProjwfcCalculation. Similarly, we can add an optional input structure for ProjwfcCalculation. But for number_of_spin_components, adding an optional input parameters containing number_of_spin_components to ProjwfcCalculation, is a bit weird. Also the projwfc.x raw output does not contain spin info.
Or, in a totally different way, the XML file in the parent_folder contains the needed info to generate kpoints, structure, and number_of_spin_components, should we parse the XML in ProjwfcParser?

@sphuber
Copy link
Contributor

sphuber commented Aug 6, 2021

In essence, the open_grid.x just read the wavefunctions of IBZ kpoints, apply some symmetry operations to generate wavefunction at the remaining kpoints. For pw.x, we can do a nscf calculation on full kmesh with restarting from IBZ calculation, but that requires diagonalization of the Kohn Sham equations, and orthogonalization of KS orbitals, this is much slower than reading and rewriting wavefunctions. This difference is something like 30 mins v.s. 3 mins.

Thanks a lot. So just to see if I get the use case here: after an NSCF performed on a set of kpoints in the IBZ, instead of doing a new NSCF restart on the full mesh before passing it to projwfc.x, we first go through open_grid.x (instead of pw.x) and then directly go to projwfc.x?

Yes I agree we should fix the ProjwfcParser, the question is how to fix it:
the ProjwfcParser requires a kpoints, a structure, and a output_parameters containing number_of_spin_components,

nspin = parent_param.get_dict()['number_of_spin_components']

In this opengrid case, the ProjwfcParser should use the opengrid output kpoints, since it is different from its input kpoints, so we can add an optional input kpoints to ProjwfcCalculation. Similarly, we can add an optional input structure for ProjwfcCalculation.

I am not sure that adding inputs to ProjwfcCalculation is the way to go. The whole problem with the ProjwfcParser that we have now is that it is not self-contained and reaches up the provenance graph to get information from preceding calculations. Passing in an output of the parent calculation as an input, is just another version of this same workaround.

But for number_of_spin_components, adding an optional input parameters containing number_of_spin_components to ProjwfcCalculation, is a bit weird. Also the projwfc.x raw output does not contain spin info.
Or, in a totally different way, the XML file in the parent_folder contains the needed info to generate kpoints, structure, and number_of_spin_components, should we parse the XML in ProjwfcParser?

If the output of projwfc.x itself does not contain the necessary information, than we should indeed get it from the XML of the parent folder because this is exactly what projwfc.x bases itself on. This would also solve the problem of using a parent_folder that may come from different calcjobs. In this way, it should not matter for the ProjwfcParser and simply always parses the same XML. That is to say that QE doesn't do something incredibly weird and annoying and have pw.x and open_grid.x write different output formats... 😅

qiaojunfeng added a commit to qiaojunfeng/aiida-wannier90-workflows that referenced this pull request Oct 5, 2021
@qiaojunfeng
Copy link
Collaborator Author

Thanks @mbercx for fixing #722, #741, and #747!
Now I have removed those strange input spec from OpengridCalculation, and added some tests.
Finally, the whole Wannier workchain runs smoothly 😄

@sphuber
Copy link
Contributor

sphuber commented Oct 6, 2021

Thanks @qiaojunfeng . I propose we wait for @mbercx 's PRs to be finished and merged and then we rebase your PR. Then I will give it a final pass and we can merge it.

Copy link
Member

@mbercx mbercx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @qiaojunfeng! Already gave the PR a quick pass, left some minor nitpicks.

I wanted to give this a test myself, also so I can see open_grid.x in action. However, you mentioned something on Slack regarding a customized version of open_grid.x that's already merged into the develop branch of QE, but not released. Does this mean that this implementation doesn't work with the latest released version of QE?

aiida_quantumespresso/calculations/opengrid.py Outdated Show resolved Hide resolved
aiida_quantumespresso/calculations/opengrid.py Outdated Show resolved Hide resolved
aiida_quantumespresso/calculations/opengrid.py Outdated Show resolved Hide resolved
aiida_quantumespresso/parsers/opengrid.py Outdated Show resolved Hide resolved
aiida_quantumespresso/parsers/opengrid.py Outdated Show resolved Hide resolved
tests/parsers/test_opengrid.py Outdated Show resolved Hide resolved
aiida_quantumespresso/parsers/opengrid.py Outdated Show resolved Hide resolved
aiida_quantumespresso/parsers/opengrid.py Outdated Show resolved Hide resolved
tests/calculations/test_opengrid.py Outdated Show resolved Hide resolved
setup.json Outdated Show resolved Hide resolved
@qiaojunfeng
Copy link
Collaborator Author

All right, just did some checks, including

  • launch OpenGridCalculation on top of the RemoteData output folder of a scf PwCalculation
  • launch ProjwfcCalculation on top of the RemoteData output folder of the OpenGridCalculation

and all things work well.

One noticeable change: I use OpenGridCalculation instead of OpengridCalculation, and quantumespresso.open_grid instead of quantumespresso.opengrid for the entry point name, to be more consistent with the QE executable name open_grid.x, as @sphuber suggested one year ago. Although the capital case and the underscore look a bit unwieldy to me, I can tolerate this for the sake of consistency.

For the compatibility matrix, maybe we can add a footnote to the QE version, saying for open_grid.x the user needs to run QE>=7.0 instead of raising the lower bound of QE version?

Copy link
Member

@mbercx mbercx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks @qiaojunfeng! Just had a quick look at the changes, and only have a few comments.

src/aiida_quantumespresso/parsers/open_grid.py Outdated Show resolved Hide resolved
@qiaojunfeng qiaojunfeng changed the title Add OpengridCalculation for open_grid.x code Add OpenGridCalculation for open_grid.x code Nov 25, 2022
@mbercx mbercx self-assigned this Jan 25, 2023
qiaojunfeng and others added 9 commits February 7, 2023 16:58
open_grid.x unfolds a scf/nscf calculation with symmetry-reduced
kpoints to the full kpoints. It rewrites the eigenvalues and
recalculates the wavefunctions.
With (aiidateam#747) the `ProjwfcParser` has no implicit requirements on
parent calculation. The code for compatibility with `ProjwfcCalculation`
is now removed.
Co-authored-by: Marnik Bercx <[email protected]>
Co-authored-by: Sebastiaan Huber <[email protected]>
Copy link
Member

@mbercx mbercx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me this one is good to go! 🚀 @sphuber did you still want to have another look or can I go ahead and merge?

@sphuber
Copy link
Contributor

sphuber commented Feb 8, 2023

Sure looks good to me! Thanks a lot to both. Just make sure to squash merge and add a nice commit message as per yuzh

@qiaojunfeng qiaojunfeng merged commit 361ff04 into aiidateam:main Feb 9, 2023
@qiaojunfeng qiaojunfeng deleted the opengrid branch February 9, 2023 18:20
bastonero pushed a commit that referenced this pull request Apr 5, 2023
A `Calculation` that wraps `open_grid.x` code.

The `open_grid.x` unfolds an scf/nscf calculation with symmetry-reduced
kpoints to the full kpoint mesh. It rewrites the eigenvalues and
rotates the wavefunctions according to symmetries.

Note this relies on a PR to QE, which is merged since QE 7.0
https://gitlab.com/QEF/q-e/-/merge_requests/1526

Co-authored-by: Marnik Bercx <[email protected]>
Co-authored-by: Sebastiaan Huber <[email protected]>
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.

3 participants