aiida-common-workflows icon indicating copy to clipboard operation
aiida-common-workflows copied to clipboard

Feature/relax and bands wc

Open bosonie opened this issue 3 years ago • 8 comments

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 avatar Dec 16 '21 10:12 bosonie

@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 avatar Dec 16 '21 10:12 bosonie

I will also now write few tests

bosonie avatar Dec 16 '21 10:12 bosonie

@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.

sphuber avatar Dec 16 '21 16:12 sphuber

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..

bosonie avatar Dec 16 '21 16:12 bosonie

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.'})

sphuber avatar Dec 17 '21 08:12 sphuber

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 avatar Dec 17 '21 09:12 bosonie

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 avatar Dec 17 '21 19:12 bosonie

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

bosonie avatar Dec 17 '21 23:12 bosonie