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

PwRelaxWorkChain: replace relaxation_scheme with relax_type #614

Merged
merged 1 commit into from Nov 19, 2020

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Nov 18, 2020

Fixes #611

The relaxation_scheme was used to communicate the type of relaxation
to be performed and accepted either relax or vc-relax, directly
modeled after the relax options that could be passed to the
CONTROL.calculation input parameter.

There are, however, many other relax modes possible, such as those
constraining parts of the degree of freedom of the cell. To support
these a new input is introduced relax_type that replaces the old
relaxation_scheme that is deprecated. The relax_type can take values
as specified by the RelaxType enum.

@sphuber sphuber force-pushed the feature/611/pw-relax-workchain-relax-type branch from f1558ad to 5e28af0 Compare November 18, 2020 11:40
@sphuber sphuber requested a review from mbercx November 18, 2020 11:40
@sphuber sphuber added the pr/on-hold PR should not be merged label Nov 18, 2020
@sphuber sphuber force-pushed the feature/611/pw-relax-workchain-relax-type branch from 5e28af0 to 66c565d Compare November 19, 2020 13:01
@sphuber sphuber removed the pr/on-hold PR should not be merged label Nov 19, 2020
@sphuber
Copy link
Contributor Author

sphuber commented Nov 19, 2020

@mbercx this is ready for review now

Copy link
Member

@mbercx mbercx left a comment

Choose a reason for hiding this comment

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

Thanks @sphuber! Just have a few minor comments/questions.

elif self.inputs.relaxation_scheme.value == 'vc-relax':
relax_type = RelaxType.ATOMS_CELL
else:
raise ValueError('insupported value for the `relaxation_scheme` input.')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
raise ValueError('insupported value for the `relaxation_scheme` input.')
raise ValueError('unsupported value for the `relaxation_scheme` input.')

aiida_quantumespresso/common/types.py Show resolved Hide resolved
aiida_quantumespresso/workflows/pw/relax.py Show resolved Hide resolved
help='The relaxation scheme to use: choose either `relax` or `vc-relax` for variable cell relax.')
spec.input('relax_type', valid_type=orm.Str, default=lambda: orm.Str(RelaxType.ATOMS_CELL),
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we change this to:

Suggested change
spec.input('relax_type', valid_type=orm.Str, default=lambda: orm.Str(RelaxType.ATOMS_CELL),
spec.input('relax_type', valid_type=orm.Str, default=lambda: orm.Str(RelaxType.ATOMS_CELL.value),

Since:

In [1]: from aiida_quantumespresso.common.types import RelaxType

In [2]: from aiida_quantumespresso.workflows.pw.relax import validate_relax_type

In [3]: validate_relax_type(Str(RelaxType.ATOMS_CELL), None)
Out[3]: '`RelaxType.ATOMS_CELL` is not a valid value of `RelaxType`.'

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 we should

The `relaxation_scheme` was used to communicate the type of relaxation
to be performed and accepted either `relax` or `vc-relax`, directly
modeled after the relax options that could be passed to the
`CONTROL.calculation` input parameter.

There are, however, many other relax modes possible, such as those
constraining parts of the degree of freedom of the cell. To support
these a new input is introduced `relax_type` that replaces the old
`relaxation_scheme` that is deprecated. The `relax_type` can take values
as specified by the `RelaxType` enum.
@sphuber sphuber force-pushed the feature/611/pw-relax-workchain-relax-type branch from 66c565d to 28f396c Compare November 19, 2020 14:13
@sphuber sphuber requested a review from mbercx November 19, 2020 14:13
Copy link
Member

@mbercx mbercx left a comment

Choose a reason for hiding this comment

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

All good, thanks @sphuber!

@sphuber sphuber merged commit 4007fd8 into develop Nov 19, 2020
@sphuber sphuber deleted the feature/611/pw-relax-workchain-relax-type branch November 19, 2020 14:22
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.

PwRelaxWorkChain: replace relaxation_scheme with relax_type to support more relax types
2 participants