-
Notifications
You must be signed in to change notification settings - Fork 79
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
: Move RelaxType
input to get_builder_from_protocol
#634
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mbercx . Have a couple of suggestions
9b74944
to
5607599
Compare
Alright, @sphuber, I made the changes and already took a soft look at some of the logic. However, I'm left wondering why we treat the
What do you think? Is there anything I've missed? I think right now it's a little confusing that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor things
98f18ee
to
8a745bb
Compare
Alright, @sphuber! As per our discussion, I've properly separated church and state by moving the
Let me know if I should reorganise the commits somewhat. Should we perhaps split this in two PR's? i.e.:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mbercx . Looking good, just two minor comments.
Let me know if I should reorganise the commits somewhat. Should we perhaps split this in two PR's?
To be honest, I see two commits at most, but for me it would be good to squash them in a single one and simply describing well all the individual changes. Most of the changes should not really affect behavior of what was released.
Uh oh! Copy Pasta! 😅 |
c89983a
to
50713cc
Compare
self.ctx.relax_inputs.pw.parameters['CONTROL']['calculation'] in ('scf', 'relax') or | ||
self.ctx.relax_inputs.pw.parameters['CELL']['cell_dofree'] == 'shape' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has still the capacity to fail now, no? I don't think it is guaranteed that ['CONTROL']['calculation']
or ['CELL']['cell_dofree']
exists. For the former, we should probably add a validator in the spec because the user now has to indicate the calculation mode (before we did it through the relax_type
input). The second one is sometimes not defined and that is ok, so we should do something like self.ctx.relax_inputs.pw.parameters.get('CELL', {}).get('cell_dofree', None) == 'shape'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, of course. 🤦
One issue with the validator is that the user might still use the relaxation_scheme
input, and I don't think I can check if this input is provided in the validator of the base
input? So the user might not provide the 'calculation'
, in the base
input, but the relaxation_scheme
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can put the validator on the top-level namespace for now with a comment that once relaxation_scheme
is removed, it can be moved to the parameters
port. In the top-level you will have access to all inputs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the validator which checks if the base
input has ['pw']['parameters']['CONTROL']['calculation']
specified. It might be a little confusing to a user that still relies on relaxation_scheme
and doesn't specify the calculation
input, but I maybe this is ok? Or is there an elegant solution to this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, as my previous comment said, you can move the validator to the top level inputs which has access to all
def validate_inputs(inputs, ctx):
"""Validator for top level namespace."""
parameters = inputs['base']['pw']['parameters'].get_dict()
if 'relaxation_scheme' not in inputs and 'calculation' not in parameters.get('CONTROL', {}):
return 'The parameters in `base.pw.parameters` do not specify the required key `CONTROL.calculation`.'
def define(cls, spec):
spec.input()
....
spec.inputs.validator = validate_inputs
Note also that you should not raise an exception from a validator but simply return a string with the error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it seems Github hadn't updated the comments when I replied to yours, sorry about that!
PwRelaxWorkChain
: Run 'scf' calculation for RelaxType.NONEPwRelaxWorkChain
: Move RelaxType
input to get_builder_from_protocol
e9672b3
to
6c2d5ed
Compare
* Set the relax_type and meta_convergence in the setup step and add it to the ctx. * Simplify the logic of the results step
Instead of having the Relaxtype as an input of the PwRelaxWorkChain, it would be better to properly separate church and state by making it an input of the `get_builder_from_protocol` method. This will properly populate the builder of the `PwRelaxWorkChain` depending on the chosen `Relaxtype`.
1748fc1
to
46a2cf6
Compare
Instead of having the Relaxtype as an input of the PwRelaxWorkChain, it
would be better to properly separate church and state by making it an
input of the
get_builder_from_protocol
method. Based on the chosenRelaxtype
, this will properly populate the builder of thePwRelaxWorkChain
.Second, currently for
RelaxType.NONE
, thePwRelaxWorkChain
fixesthe atoms using the
if_pos
inputs explained explained here:https://www.quantum-espresso.org/Doc/INPUT_PW.html#idm1094
and then run a
'relax'
type calculation. This leads to issues withall calculations raising
ERROR_IONIC_CONVERGENCE_NOT_REACHED
exit codes from the parser.
Instead, here we set
parameters['CONTROL']['calculation'] = 'scf'
forRelaxType.NONE
in theget_builder_from_protocol
method, sincethis "relaxation" type really corresponds to a static calculation.
Besides this, we also:
PwRelaxWorkChain
only runs 1PwBaseWorkChain
forRelaxType
'sNONE
,ATOMS
andSHAPE
. I.e. meta convergence isturned off.
'calculation'
is set to'scf'
for the
PwBaseWorkChain
that does the relaxation.results
step of the outlinewhen running with
'calculation'
set to'scf'
.TODO
as
PwRelaxWorkChain
s.RelaxType
s and the work chain logic.RelaxType
s and make sure we don't run into more errors.