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

PwBandStructureWorkChain: Deprecate and fix #688

Merged

Conversation

mbercx
Copy link
Member

@mbercx mbercx commented May 7, 2021

Fixes #687

The PwBandStructureWorkChain is a legacy version of a band structure work
chain with a pre-defined protocol, mostly used for previous tutorials.
However, now that the PwBandsWorkChain has been equipped with a protocol
through the get_builder_from_protocol() method, it no longer serves a
purpose and should be deprecated.

However, there were still two issues with running the work chain that
needed to be fixed, since users were still running it and it is still
supported until v4.0.0:

  • The get_pseudos_from_dict() method, used by the work chain, only
    accepted UpfData from the aiida-pseudo package. Since the work chain
    was often run with legacy UpfData, this raised errors for several
    users. This is fixed by also accepting the legacy UpfData here.
  • There was a small bug in the PwRelaxWorkChain when using the
    relaxation_scheme input (which is also deprecated). Basically, at some
    point in the work chain, the base input port was checked for the
    pw.parameters.CONTROL.calculation value, but in case the user doesn't
    specify this input (which passes validation when the relaxation_scheme
    is provided), this would raise a KeyError when running the work chain.
    This is fixed by obtaining the calculation value from the context
    instead.

The `PwBandStructureWorkChain` is a legacy version of a band structure work
chain with a pre-defined protocol, mostly used for previous tutorials.
However, now that the `PwBandsWorkChain` has been equipped with a protocol
through the `get_builder_from_protocol()` method, it no longer serves a
purpose and should be deprecated.

However, there were still two issues with running the work chain that
needed to be fixed, since users were still running it and it is still
supported until v4.0.0:

* The `get_pseudos_from_dict()` method, used by the work chain, only
accepted `UpfData` from the `aiida-pseudo` package. Since the work chain
was often run with legacy `UpfData`, this raised errors for several
users. This is fixed by also accepting the legacy `UpfData` here.
* There was a small bug in the `PwRelaxWorkChain` when using the
`relaxation_scheme` input (which is also deprecated). Basically, at some
point in the work chain, the `base` input port was checked for the
`pw.parameters.CONTROL.calculation` value, but in case the user doesn't
specify this input (which passes validation when the `relaxation_scheme`
is provided), this would raise a `KeyError` when running the work chain.
This is fixed by obtaining the `calculation` value from the context
instead.
@mbercx mbercx requested a review from sphuber May 7, 2021 17:06
@mbercx
Copy link
Member Author

mbercx commented May 7, 2021

@sphuber do I still have to add tests for this, since the inputs related to the bugs have been deprecated anyways? 😬

@sphuber
Copy link
Contributor

sphuber commented May 7, 2021

@sphuber do I still have to add tests for this, since the inputs related to the bugs have been deprecated anyways? grimacing

If you have tested it locally and verified that it works, it is fine to omit the unit tests for this legacy workchain

@mbercx
Copy link
Member Author

mbercx commented May 7, 2021

If you have tested it locally and verified that it works, it is fine to omit the unit tests for this legacy workchain

I followed the tutorial instructions and: 🙃

$ verdi process status 527
PwBandStructureWorkChain<527> Finished [0] [3:results]
    └── PwBandsWorkChain<541> Finished [0] [7:results]
        ├── PwRelaxWorkChain<543> Finished [0] [3:results]
        │   ├── PwBaseWorkChain<546> Finished [0] [6:results]
        │   │   ├── create_kpoints_from_distance<548> Finished [0]
        │   │   └── PwCalculation<552> Finished [0]
        │   └── PwBaseWorkChain<561> Finished [0] [6:results]
        │       ├── create_kpoints_from_distance<563> Finished [0]
        │       └── PwCalculation<567> Finished [0]
        ├── seekpath_structure_analysis<574> Finished [0]
        ├── PwBaseWorkChain<581> Finished [0] [6:results]
        │   ├── create_kpoints_from_distance<583> Finished [0]
        │   └── PwCalculation<587> Finished [0]
        └── PwBaseWorkChain<595> Finished [0] [6:results]
            └── PwCalculation<598> Finished [0]

@sphuber
Copy link
Contributor

sphuber commented May 7, 2021

I followed the tutorial instructions and: upside_down_face

That's all one can ask for 👍

@sphuber sphuber merged commit 2cea42a into aiidateam:develop May 7, 2021
@mbercx mbercx deleted the fix/687/pwbandstructure-workchain branch May 7, 2021 18:09
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.

PwBandStructureWorkChain: Deprecate and fix issues
2 participants