Implement CommonBandsWorkChain for CASTEP #258
Conversation
| @@ -32,7 +32,6 @@ def define(cls, spec): | |||
| ) | |||
| spec.input( | |||
| 'protocol', | |||
| valid_type=ChoiceType(('fast', 'moderate', 'precise')), | |||
There was a problem hiding this comment.
Good to discuss whether this should be limited, but this explicit definition did have the advantage that it made it discoverable exactly which protocols are available. If we remove this, we should reintroduce it in some other way.
There was a problem hiding this comment.
This is an interesting point. The valid protocols can be always extended (actually changed) inside the code implementations of the CommonRelaxInputGenerator, am I right @sphuber? This is compatible also with the concept of creating code-agnostic workchains only if we remove the choice check in the inputs of the workchain. Foor instance this is what we do in the EOS wc: the valid types disappear (
There was a problem hiding this comment.
good point @bosonie. I should be able to just override it in CastepCommonRelaxInputGenerator. Will drop the changes to the base class.
| """Extract the Fermi energy from the BandsData output""" | ||
| efermi = bands.get_attribute('efermi') | ||
| if isinstance(efermi, list): | ||
| efermi = efermi[0] |
There was a problem hiding this comment.
This is a good point. Should we discuss whether the fermi_energy output should be a Float or a List[float] depending on polarization? Think this would be a common "problem" across implementations
There was a problem hiding this comment.
I will put in the list for discussion tomorrow
There was a problem hiding this comment.
An additional issue is that some code may not have a well defined single efermi energy if there are two spin channels (is such quantity itself well-defined at all?). Not sure if this is a limitation across the board or not. I know CASTEP always give two if there are two spin channels. VASP gives ones fermi energy with spin polarised calculation I think, but it may give a wrong value if spin is constrained.
| else: | ||
| builder_common_bands_wc.structure = parent_castep_calc.inputs.structure | ||
|
|
||
| engb = engines['bands'] |
There was a problem hiding this comment.
This will except if engines was not specified in kwargs
| engb = engines['bands'] | ||
| builder_common_bands_wc.scf.calc.code = engines['bands']['code'] | ||
| if 'options' in engb: | ||
| builder_common_bands_wc.scf.calc_options = orm.Dict(dict=engines['bands']['options']) |
There was a problem hiding this comment.
I think you may have just copied this from the Siesta implementation. Does the Castep implementation also expect the options as a Dict node as opposed to just a dictionary in the metadata namespace? If so, I would say that is suboptimal and we should think of improving that in aiida-castep at some point.
There was a problem hiding this comment.
CASTEP actually work with metadata exposed from CalcJob previously, e.g. one pass the metadata.options as a dict not Dict, to an exposed calc.metadata port namespace.
I just updated the code to make it work with Dict...
Which way do you think is better? For the plain metadata.options approach the problem I had is that one cannot just do get_builder_restart from a WorkChainNode and have the builder fully populated and ready to go. This is because the exposed metadata.options namespace remains empty.
There was a problem hiding this comment.
I see the problem with the options not being stored, but I am personally not a fan of closing of the original port and replacing it with a custom Dict analog. Rather, I think we should solve the root of the issue and see if there is a way that the options can be stored on the workflow node.
There was a problem hiding this comment.
I see, I will just keep both ways acceptable by aiida-castep for now.
Only apply a minimum cut off if cut off energy is not explicitly set already.
The Y and No elements expanded into Y and No....
Added modified pseudopotentials for lanthides and actinides.
Adds implementation of the bands workchain for CASTEP plugin.
I made a custom protocol for the oxide work. Make relaxation input generator accepts, checks on the protocol choices should be removed. I think it would be a good thing to have it removed any way - so we don't limit ourselves to three protocols only?