Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement CommonBandsWorkChain for FLEUR #259

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

janssenhenning
Copy link
Contributor

@janssenhenning janssenhenning commented Jan 14, 2022

Adds the FleurCommonBandsWorkChain and input generator.

One note:

  • I noticed while testing, that the engines input cannot be omitted as the get_builder() method will raise an error that engines.bands.code is missing. But the implementations I used as a reference seem to handle this case at least partially. Is this wanted behaviour?

@janssenhenning janssenhenning changed the title Add CommonBandsWorkChain implementation for FLEUR Implement CommonBandsWorkChain for FLEUR Jan 14, 2022
@janssenhenning
Copy link
Contributor Author

@bosonie @sphuber I, unfortunately, cannot request a review directly so I'm pinging here. I think to request a review I also need to be approved first.

Copy link
Collaborator

@sphuber sphuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks fine to me, just have a comment that relates to the implementation of the fleur workchains. Also you are right, the engines input to the generator is optional and the other implementations do not properly check for this. I am not sure whether this should remain optional or whether it should become required.

Comment on lines +43 to +44
builder.options = orm.Dict(dict=builder_parent.metadata.options)
builder.fleur = builder_parent.code
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really a point for this PR, but I don't think it is advisable to have the Code and options inputs a separate ports. And especially exposing the options as a Dict node instead of a normal dict in the metadata of the calculation job is subideal. Is there a reason why you do this and you don't let the caller specify this information in the exposed inputs of the FleurScfWorkChain?

Taking a closer look, it seems that this is just a continuation problem of the fact that FleurScfWorkChain doesn't expose the inputs of FleurBaseWorkChain but manually creates input ports. If possible, I would recommend to properly use expose_inputs when wrapping subworkflows.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are right we have definitely underused exposing the inputs/outputs in the aiida-fleur plugin. I've been trying to expand the usage of this, where it leads to no breaking changes in the last months. One slight problem is that on the level of the FleurSCFWorkChain we have two different codes that can potentially be used. The fleur code and the inpgen code, which is used when we start a calculation from a structure input.

We can probably move to exposing the inputs by exposing both the FleurBaseWorkChain and the FleurinputgenCalculation, with one or both being in a namespace. But since the scf workchain is the main workchain that is then used throughout almost all higher level workchains this is quite a large breaking change, so we need to be careful with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants