Allow null pseudo_family by extracting z_valence from pseudos

[edan-bainglass]

In the pw base workchain's get_builder_from_protocol method, we use the pseudo_family input (str) to fetch the family and use it to define defaults for pseudos and cutoffs, as well as to obtain the Z value for converting moments to starting magnetization. This PR removes the dependency on pseudo_family by relying on the overrides for pseudos and cutoffs (now required if no pseudo family provided), as well by constructing a z_valences dictionary from the pseudos themselves and using it in the magnetization conversion. By removing the dependency, this PR allows scenarios where pseudos are provided that do not belong to the supported families (e.g., uploading custom pseudos in the QE app).

[mbercx]

Thanks @edan-bainglass! Is this urgent? I'd like to have a think on how to best deal with custom pseudo potentials. I.e. perhaps it should be easier to define a PseudoPotentialFamily through aiida-pseudo and run that. Or maybe it makes more sense to just adapt the pseudo potentials, cutoffs etc after obtaining the builder?

Could you also point me to an example how this new feature is used?

[edan-bainglass]

Thanks @edan-bainglass! Is this urgent? I'd like to have a think on how to best deal with custom pseudo potentials. I.e. perhaps it should be easier to define a PseudoPotentialFamily through aiida-pseudo and run that. Or maybe it makes more sense to just adapt the pseudo potentials, cutoffs etc after obtaining the builder?

Could you also point me to an example how this new feature is used?

Hi @mbercx. See referenced PR from the QE app. Urgent enough that its pseudo upload widget wasn't actually an operational feature. That PR addresses the issue (restores compatibility), but to submit successfully, this PR would be required.

Would it be possible to get this merged and think of an improved/optimal implementation later?

[t-reents]

@edan-bainglass
I had a look as well and the changes generally look fine for me.

Just some general comments from my side:

I.e. perhaps it should be easier to define a PseudoPotentialFamily through aiida-pseudo and run that.

Since I have been playing around with several pseudos lately, where I was only changing individual ones and didn't care about a whole PP library/family, I would prefer to not add this additional step of forcing the user to first use aiida-pseudo, if I understood your comment correctly.

Or maybe it makes more sense to just adapt the pseudo potentials, cutoffs etc after obtaining the builder?

Wouldn't this somehow counteract the idea of the builder that you only have to call a single function to get a final builder, potentially including custom modifications based on the provided overrides. Just to say, I personally find it a bit odd to remove certain aspects from the overrides (again, assuming that I understood your comment correctly), e.g., that I can't directly modify the pseudos etc. in the builder, but everything else.

[t-reents]

@edan-bainglass Ah, final comment, I think that we should at least add/adjust tests to check that this new logic works as expected, especially if we want to merge it now and think about an optimal UX later.

[mbercx]

Thanks @t-reents, I agree with both your comments. I think I was mainly giving some off the cuff comments to try and procrastinate on the pseudo overrides issue. ^^ But let's bite the bullet.

Having a closer look now, this PR is indeed trying to fix some bugs with overriding pseudos in the get_builder_from_protocol method that have not been noticed since we've relied on PseudoPotentialFamily so much:

1. If the user overrides the pseudos input, the starting_magnetization etc is still constructed taking the z_valence from the pseudo_family.
2. If the user overrides the pseudos input but doesn't override the cutoffs, these are still (silently) taken from the pseudo_family.

However, in the current implementation, these issues still (silently) persist unless the user explicitly sets the pseudo_family input to None or False. In the QEapp this is probably taken care of, but I think that for other aiida-quantumespresso users this won't be intuitive. They'll try to override the pseudos input without touching pseudo_family, and potentially get e.g. an incorrect starting_magnetization. I would try to fix that here.

@t-reents since you've playing around with custom pseudos outside the scope of the QEapp: one question I'm still thinking about: should the user be able to override the pseudo potential for one kind and still take the others from the pseudo family?

A second change I'd make is to the get_magnetization function. Now the API is a bit strange: the pseudo_family has no default, but is an optional input. I'd just replace the pseudo_family input by z_valences to make it more generic. Since we haven't released it yet, we can be backwards incompatible.

@edan-bainglass since I'm introducing a bit of scope creep here, happy to take over to get these bug fixes in for all aiida-quantumespresso users. I'll also take care of adding tests then.

[edan-bainglass]

@edan-bainglass since I'm introducing a bit of scope creep here, happy to take over to get these bug fixes in for all aiida-quantumespresso users. I'll also take care of adding tests then.

Sure @mbercx. Can you please provide a rough estimate for completion on this? Reminder that aiidalab/aiidalab-qe#1348 is blocked by this 🙂

[mbercx]

I'll work on it first thing tomorrow Aussie time, so it will be ready for you to test in the QEapp by the time you have your morning coffee. 😉

[mbercx]

@edan-bainglass I've added some basic tests (to be condensed) in this PR to demonstrate the two issues with overriding pseudo potentials, and that these are resolved in case the pseudo_family is set to None. So the tests should fail now.

I'll work on adapting the code so all these tests pass in a separate PR.

EDIT: Working on new version in #1102

Since the tests will be refactored in that PR, I show that the changes pass the tests I introduced in this PR here:

https://github.com/mbercx/aiida-quantumespresso/actions/runs/16612834078/job/46999197620

The old test_pseudos_overrides now fails, but it should. In case the user overrides the pseudo potentials, they should also provide the cutoffs. Taking the cutoffs from the default pseudo_family silently is a bad idea.

[mbercx]

Superseded by #1102. Thanks again @edan-bainglass for finding this issue and the fix!