馃悰 get_builder_from_protocol: Fix bugs when overriding pseudopotentials - #1102
Conversation
The current `get_magnetization` utility function has an input `pseudo_family` that expects an `aiida-pseudo` `PseudoPotentialFamily` to extract the number of valence electrons in the pseudo potential. However, when a user wants to use custom pseudo potentials e.g. through the `overrides` of the `get_builder_from_protocol()` method, it would still be beneficial to use the `get_magnetization` function to determine the correct Quantum ESPRESSO inputs. Here we make the function more generic by replacing the `pseudo_family` input by `z_valences`, a simple dictionary that maps each kind name to the corresponding number of valence electrons in the pseudo potential. Co-authored-by: Edan Bainglass <edan.bainglass@gmail.com>
The `generate_upf_data` is used to generate a minimal `UpfData` node. Currently it only allows you to determine the element of the pseudo potential. Here we introduce a new `z_valence` input to the fixture for setting the number of valence electrons.
There are currently two bugs in the `PwBaseWorkChain.get_builder_from_protocol()` method when the users overrides the `pw.pseudos` input: 1. For magnetic calculations, the `starting_magnetization` is still constructed based on the number of valence electrons (`z_valence`) from the `pseudo_family`. 2. If the user overrides the `pseudos` input but doesn't override the cutoffs (`SYSTEM.ecutrho`, `SYSTEM.ecutwfc`), these are still (silently) taken from the `pseudo_family`. Here we fix these two issues by 1. Passing the `z_valences` of the provides `pseudos` override to the `get_magnetization` function. 2. Strictly checking the overrides: they should both (i) provide pseudo potentials for _all_ the kinds in the input `structure` and (ii) provide both energy cutoffs (`SYSTEM.ecutrho`, `SYSTEM.ecutwfc`) in case the `pseudos` are overridden. Moreover, if the user completely specifies the pseudos and cutoffs through the overrides, it should be fine in case `pseudo_family` is None or False. This is important in the context of adding custom pseudo potential functionality in the QEapp, see: aiidalab/aiidalab-qe#1348 Co-authored-by: Edan Bainglass <edan.bainglass@gmail.com>
|
@edan-bainglass I added specific tests to make sure the |
|
Mainly to keep your branch + commits in place, along with the test runs. Happy to force push to your branch, but is there a reason why you'd prefer that? |
I think I'm under the impression that building your changes as an extension on my PR would achieve this. Are you suggesting this is not true, and to do so, it is required to make a separate PR? I'm missing something 馃槄 In any case, this is not an issue. I was just confused at the path taken. Since the changes are the same, this is easy to review 馃槄 Let's get this merged asap, so we can proceed with aiidalab/aiidalab-qe#1348. Should I close #1100 then? |
edan-bainglass
left a comment
edan-bainglass
left a comment
There was a problem hiding this comment.
LGTM! Familiar even 馃槄 One comment regarding the get_magnetization utility - in #1100, I kept pseudo_family for backward compatibility. You remove it here in favor of z_valences. Are you confident this is not an issue?
|
Cheers @edan-bainglass. Apologies if my approach was confusing, I simply didn't know if you'd be happy with me force-pushing your branch, and wanted to split the commits up for clarity. Re I'm going to give @t-reents a chance to review, as a regular |
All good 馃檪
Ah, I missed that comment. Okay, got it 馃憤
Sounds good 馃憤 Thanks for the work @mbercx 馃檹 |
|
Thanks @mbercx ! Looks good to me! I like the additional validation that ensures that one provides cutoffs etc. While this is something I was doing before, I also missed the fact that the You've already asked in #1100 whether it should be possible to only override individual pseudos. While I'd potentially say yes in certain scenarios, e.g. you have a structure with many elements and you want to see what happens if you change the pseudo for a single element (you now have to get the other pseudos from SSSP first), I think that the current version of the PR is fine, since it definitely avoids bugs etc., and reminds the user to think about these aspects. |
|
Thanks @t-reents for the review, and @edan-bainglass for the OG fix in #1100! Will rebase + merge, close that PR and open a release PR momentarily! |
3 participants
As mentioned in this comment, #1100 highlighted two issues with the current logic for using custom pseudo potentials through the
overridesinput of theget_builder_from_protocol()method:pseudosinput, thestarting_magnetizationetc is still constructed taking thez_valencefrom thepseudo_family.pseudosinput but doesn't override the cutoffs, these are still (silently) taken from thepseudo_family.Here we try to fix these two issues. Moreover, if the user completely specifies the
pseudosand cutoffs through the overrides, it should be fine in casepseudo_familyisNoneorFalse. This is important in the context of adding custom pseudo potential functionality in the QEapp: aiidalab/aiidalab-qe#1348.