search

aiidateam / aiida-quantumespresso

The official AiiDA plugin for Quantum ESPRESSO
https://aiida-quantumespresso.readthedocs.io
Other
53 stars 78 forks source link

Feat: pop None inputs specified in overrides #1022

Open bastonero opened 6 months ago

bastonero commented 6 months ago

Fixes #653

Add the possibility of popping input namespaces by specifying None in the override for the specific namespace. A decorator is added that generalize the concept to any implementation of get_builder_from_protocol.

bastonero commented 6 months ago

Hi @mbercx @sphuber , it is still somehow draft mode, but I think the basics are there. It works nicely for the test I have written. The limitation of course is that it cannot pop keys within Dict inputs (but I guess that's fine?). I'd love to have your inputs and thoughts.

Fun note: the decorator comes actually from a question to ChatGPT+Copilot, as I just bought it and I was hyper-curious to see the capabilities. Impressive to see what one can achieve with a couple of questions!

mbercx commented 5 months ago

Thanks @bastonero! But I'm afraid at first glance I'm not too happy with the solution provided by ChatGPT. ^^ Having to wrap every get_builder_from_protocol() method seems tedious, it should just be built into the ProtocolMixin.

Rough outline for what I was thinking (untested or well thought through):

Ideally, we'd first extend the protocol concept to our CalcJobs. No reason you shouldn't be able to define a protocol for a CalcJob. Then, the CalcJob is effectively the "end of the line", i.e. it can't wrap other processes. So at that point you can look for None values and remove them when merging the protocol with the overrides.

The limitation of course is that it cannot pop keys within Dict inputs (but I guess that's fine?).

This limitation is not so nice, I think. I should be able to "unset" an input as well. Even though you can probably always just set the input you want instead, sometimes the final inputs won't make sense to a user. An example would be degauss. Why would this be set if no smearing is used? Now, QE probably doesn't care (though it should), but the input being there might be confusing and just clutters the input file.

EDIT: I'd be happy to work on this, but am currently still too bogged down with other tasks to dedicate any serious time to aiida-quantumespresso. Hopefully I'll be able to wrap up you-know-what soon and really start maintaining the aiida-quantumespresso plugin again.

bastonero commented 4 months ago

Hi @mbercx , thanks for the feedback!

The idea behind this implementation is that you might have a workflow that would perform optional steps. For instance, the PwRelaxWorkChain by default would perform the base_final_scf. In this case, one needs to "pop" it from the builder by hand. This is sub-ideal if we would like to have a new cmdline base on overrides (which I have privately). So for all these cases, one simply needs to put on top the decorator. I don't know if this is possible to do already in the ProtocolMixin, but you get the idea why this is desirable.