aiidateam / aiida-common-workflows

A repository for the implementation of common workflow interfaces across materials-science codes and plugins
https://aiida-common-workflows.readthedocs.io
MIT License
52 stars 31 forks source link

Feature/relax and bands wc #254

Open bosonie opened 2 years ago

bosonie commented 2 years ago

This PR introduce a code agnostic workchain that performs relaxation and subsequently the calculation of bands. It also has the possibility to use seekpath to generates the high symmetries kpoints path for bands. This process might change the structure, therefore, when seekpath is called, a further scf step is needed on the nex structure. This is automatically handled by the workchain. The inputs for the further scf are exposed. They are the inputs of a CommonRelaxInpuGenerator, but where the default relaxation_type is NONE and all the other defaults have been removed. If not defined by user, the same inputs of the first relaxation are used (except the new default for relaxation_type).

Limitation of the current implementation (will be improved later): 1) For the moment the inputs of relaxation and bands calculations are the same as the inputs of CommonRelaxInputGenerator and CommonBandsInputGenerator respectively and they are non_db. So their value will not be stored as inputs of the workchain. 2) For the moment it is not possible to run relaxation with one plugin and bands with another. 3) The possibility of overrides to the code-dependent inputs is not possible for the moment. Discussions are ongoing on how to improve it.

bosonie commented 2 years ago

@sphuber I would like especially a feedback on how to handle defaults of the second_relax_inputs. I thought that using populate_defaults = False was enough for the exposed inputs to forget the previous defaults. This is not the case. I had to manually force them to be () which is equal to UNSPECIFIED in the current plumpy implementation (https://github.com/aiidateam/plumpy/blob/develop/plumpy/ports.py). Is this correct? Seems fragile.

bosonie commented 2 years ago

I will also now write few tests

sphuber commented 2 years ago

@sphuber I would like especially a feedback on how to handle defaults of the second_relax_inputs.

Why do you want to get rid of the defaults?

I thought that using populate_defaults = False was enough for the exposed inputs to forget the previous defaults.

Nope, that is not what it is supposed to do. Setting populate_defaults = False when exposing the inputs of a process is only used when the process is actually instantiated. This calls PortnameSpace.pre_process which will normally fill in the defaults in the inputs it received for the ports that were not explicitly specified. But for some cases this is not what you want. If you make a subprocess completely optional and rely on determining whether it should be run based on whether any inputs were defined, you don't want the defaults to be automatically added, because this means it will always be run, even if the user didn't specify anything.

bosonie commented 2 years ago

If you make a subprocess completely optional and rely on determining whether it should be run based on whether any inputs were defined, you don't want the defaults to be automatically added, because this means it will always be run, even if the user didn't specify anything.

This is exactly what I want. But unfortunately what I see is different. I set all the inputs to have populate_defaults = False but the inputs are filled in anyway when the process is instantiated! Is it again a misbehavior of the particular implementation we have here? I investigate more..

sphuber commented 2 years ago

This is exactly what I want. But unfortunately what I see is different. I set all the inputs to have populate_defaults = False but the inputs are filled in anyway when the process is instantiated! Is it again a misbehavior of the particular implementation we have here? I investigate more..

But populate_defaults is a property of the namespace not the port. You are now setting it to the ports. You are also still using the manual create_namespace and absorb. We already discussed during the coding week that you should just use expose_inputs. There you can pass the populate_defaults in the namespace options, i.e.:

spec.expose_inputs(CommonRelaxInputGenerator, namespace='relax', exclude=('structure',),
            namespace_options={'required': False, 'populate_defaults': False,
            'help': 'Inputs for the `CommonRelaxInputGenerator`, if not specified at all, the relaxation step is skipped.'})
bosonie commented 2 years ago

Ok, thanks @sphuber, I understand, but then this does not cover my use case, since I want one input in the namespace to be populated, and the others no. How do you do that? I will change to expose_inputs in the next commit

bosonie commented 2 years ago

I addressed almost all your comments @sphuber. Two are open for discussion. Thanks for your review. However I found another problematic behavior of the ports. It seems to me that the populate_default = False is ignored in some cases. I will report about that somewhere as soon as I understood correctly. So you can comment on the new implementation, but even if it is ok, do not merge please.

bosonie commented 2 years ago

The issue I found is here https://github.com/aiidateam/aiida-core/issues/5286