PwBaseWorkChain: make magnetism from overrides absolute

[mbercx]

Fixes #729

When using the get_builder_from_protocol() method from the
PwBaseWorkChain, setting spin_type to COLLINEAR while not
specifying the initial_magnetic_moments will result in a set of
default magnetic moments being used, based on the magnetization.yaml
file. This means that even when SYSTEM.starting_magnetization is
specified in the overrides, these are simply ignored in favor of the
defaults.

Here we make the overrides absolute, i.e. the
starting_magnetization is only set to the default in case it is not
specified in the overrides. This is once again based on the principle
that we should avoid quietly overriding inputs that the user explicitly
specified.

[mbercx]

@sphuber there is of course also the case where the user specifies both SYSTEM.starting_magnetization in the overrides as well as initial_magnetic_moments. Not sure what should take precedence in this case, but there should definitely be a warning.

Alternatively, we enforce that the user relies on initial_magnetic_moments, raising in case SYSTEM.starting_magnetization is specified. The benefit here would be that there is one straightforward way of adding the initial magnetic configuration, and there can be no conflicts. Downside is that we lose some flexibility, and some users might be used to magnetization instead of magnetic moments.

[mbercx]

Also pinging @tsthakur to see if he has comments. @sphuber is it ok if I give him "Triage" access to the repository so he can review PR's?

[sphuber]

Also pinging @tsthakur to see if he has comments. @sphuber is it ok if I give him "Triage" access to the repository so he can review PR's?

Sure, but maybe best he doesn't merge until he is up to speed of the procedure

[mbercx]

Sure, but maybe best he doesn't merge until he is up to speed of the procedure

Indeed, I had a look at the different permission levels, and "Triage" peeps can't merge PR's, as far as I can tell.

https://docs.github.com/en/organizations/managing-access-to-your-organizations-repositories/repository-permission-levels-for-an-organization#repository-access-for-each-permission-level

[sphuber]

Not sure what should take precedence in this case, but there should definitely be a warning.

I would say that we can just let initial_magnetic_moments take precedence because that feels a lot more explicit. I mean if you set that, and you also have something in the overrides, I don't think you can be very surprised if the former takes precedence. So I would keep things as you have.

[sphuber]

Changes looking good, but uhm

[mbercx]

Haha, of course, always love adding tests, but do you still have comments on these points?

[sphuber]

Haha, of course, always love adding tests, but do you still have comments on these points?

I thought I did? In any case, I think initial_magnetic_moments should simply override all else and I don't think we need to have warnings if it overrides something. It should be plainly obvious that if you set that one, it is going to override any custom settings in the parameter inputs.

[mbercx]

I thought I did? In any case, I think initial_magnetic_moments should simply override all else and I don't think we need to have warnings if it overrides something. It should be plainly obvious that if you set that one, it is going to override any custom settings in the parameter inputs.

Oh wow, I clearly was too focused on the meme 😅 ! Note that the parameter inputs would only have custom settings at this point in case they were provided through the overrides input. So it would def be weird to set both, maybe even indicate that the user is doing something wrong in their setup. I'm not sure if I agree that a warning is out of place here.

[sphuber]

So it would def be weird to set both, maybe even indicate that the user is doing something wrong in their setup. I'm not sure if I agree that a warning is out of place here.

It's just that I think this can easily get out of hand: there will be quite a number of arguments that can clash with values specified in the overrides. Think that we just best explain this once in the documentation that arguments passed to get_builder_from_protocol are leading and can override the overrides. Anyway, I think we agreed sometime ago that we think the concept of overrides should go away since it is confusing and it can be done "manually" directly on the builder afterwards. Since there is not additional functionality in the override system, removing the complexity is better. Of course that is different from having a switch that allows to override protocol settings. In that case there is a benefit, since a single setting can be unfolded into multiple inputs spread over the namespace and so changing just this one is more convenient than a bunch manually (think pseudo_family).

[mbercx]

@sphuber I agree that perhaps trying to check all the user inputs for possible conflicts is perhaps not the way to go. I'm not sure I fully remember the discussion of removing the overrides... Probably you are referring to #712? I do see some merit in clarifying this distinction between true workchain inputs that can be overridden after obtaining the builder, and meta inputs like the pseudo family or conv_thr_per_atom that are used by the get_builder_from_protocol() method to obtain the true inputs based on the structure. We'd have to allow some other way of specifying these meta parameters if we remove the overrides. This will require some more discussion though, so I think we can agree that is beyond the scope of this PR. 😉

[sphuber]

No indeed, this is definitely beyond the scope of this PR and I wasn't suggesting any scope screep whatsoever, whoever subtle you may have think it to have been ;) I was indeed referring to the meta inputs that I think should be overridable because there there is additional value. I think we would probably want to allow to do this in two ways:

1. by letting a user define their own protocol file
2. changing individual protocol keywords through an argument to get_builder_from_protocol, e.g., something like protocol_overrides.

We had already discussed this in one of your other PRs about reusing our infrastructure to parse protocol files and make it more flexible of where they are stored. I think #712 was not designed for this problem, but is a bit of a symptom. If there isn't a dedicated issue yet, I think I will make one soon.

[mbercx]

We had already discussed this in one of your other PRs about reusing our infrastructure to parse protocol files and make it more flexible of where they are stored. I think #712 was not designed for this problem, but is a bit of a symptom. If there isn't a dedicated issue yet, I think I will make one soon.

Right, I remember now: #678. Should probably read that discussion again. :)

In the meanwhile: What do you think of my lovely tests? :D

[sphuber]

Thanks, all good, just some minor things