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 CASTEP #258

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Empty file.
71 changes: 71 additions & 0 deletions aiida_common_workflows/workflows/bands/castep/generator.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
# -*- coding: utf-8 -*-
"""Implementation of `aiida_common_workflows.common.bands.generator.CommonBandsInputGenerator` for CASTEP."""
from aiida import engine, orm

from aiida_common_workflows.generators import CodeType

from ..generator import CommonBandsInputGenerator

__all__ = ('CastepCommonBandsInputGenerator',)


class CastepCommonBandsInputGenerator(CommonBandsInputGenerator):
"""Generator of inputs for the CastepCommonBandsWorkChain"""

@classmethod
def define(cls, spec):
"""Define the specification of the input generator.

The ports defined on the specification are the inputs that will be accepted by the ``get_builder`` method.
"""
super().define(spec)
spec.inputs['engines']['bands']['code'].valid_type = CodeType('castep.castep')

def _construct_builder(self, **kwargs) -> engine.ProcessBuilder:
"""Construct a process builder based on the provided keyword arguments.

The keyword arguments will have been validated against the input generator specification.
"""
# pylint: disable=too-many-branches,too-many-statements,too-many-locals
engines = kwargs.get('engines', None)
parent_folder = kwargs['parent_folder']
bands_kpoints = kwargs['bands_kpoints']

# From the parent folder, we retrieve the calculation that created it. Note
# that we are sure it exists (it wouldn't be the same for WorkChains). We then check
# that it is a CastepCalculation and create the builder.
parent_castep_calc = parent_folder.creator
if parent_castep_calc.process_type != 'aiida.calculations:castep.castep':
raise ValueError('The `parent_folder` has not been created by a CastepCalculation')
builder_castep_calc = parent_castep_calc.get_builder_restart()

# Construct the builder of the `common_bands_wc` from the builder of a CastepCalculation.
builder_common_bands_wc = self.process_class.get_builder()
builder_common_bands_wc.scf.calc_options = orm.Dict(dict=dict(builder_castep_calc.metadata.options))
# Ensure we use castep_bin for restart, instead of the check file
#builder_common_bands_wc.scf.options = orm.Dict(dict={'use_castep_bin': True})

builder_castep_calc.metadata = {}

# Attach inputs of the calculation
for key, value in builder_castep_calc.items():
if value and key not in ['metadata', 'structure']:
builder_common_bands_wc.scf.calc[key] = value

# Updated the structure (in case we have one in output)
if 'output_structure' in parent_castep_calc.outputs:
builder_common_bands_wc.structure = parent_castep_calc.outputs.output_structure
else:
builder_common_bands_wc.structure = parent_castep_calc.inputs.structure

engb = engines['bands']
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will except if engines was not specified in kwargs

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'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see, I will just keep both ways acceptable by aiida-castep for now.


# Set the `bandskpoints` and the `parent_calc_folder` for restart
builder_common_bands_wc.bands_kpoints = bands_kpoints
builder_common_bands_wc.scf.continuation_folder = parent_folder
builder_common_bands_wc.run_separate_scf = orm.Bool(False)

return builder_common_bands_wc
41 changes: 41 additions & 0 deletions aiida_common_workflows/workflows/bands/castep/workchain.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# -*- coding: utf-8 -*-
"""Implementation of `aiida_common_workflows.common.relax.workchain.CommonRelaxWorkChain` for CASTEP."""
from logging import getLogger

from aiida.engine import calcfunction
from aiida.orm import Float
from aiida.plugins import WorkflowFactory

from ..workchain import CommonBandsWorkChain
from .generator import CastepCommonBandsInputGenerator

__all__ = ('CastepCommonBandsWorkChain',)


@calcfunction
def get_fermi_energy(bands):
"""Extract the Fermi energy from the BandsData output"""
efermi = bands.get_attribute('efermi')
if isinstance(efermi, list):
efermi = efermi[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I will put in the list for discussion tomorrow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

logger = getLogger(__name__)
logger.warning('Spin polarised calculation - using the efermi energy of the first spin channel.')
return Float(efermi)


class CastepCommonBandsWorkChain(CommonBandsWorkChain):
"""Implementation of `aiida_common_workflows.common.bands.workchain.CommonBandsWorkChain` for CASTEP."""

_process_class = WorkflowFactory('castep.bands')
_generator_class = CastepCommonBandsInputGenerator

def convert_outputs(self):
"""Convert the outputs of the sub workchain to the common output specification."""
if 'band_structure' not in self.ctx.workchain.outputs:
self.report('CastepBandsWorkChain concluded without returning bands!')
return self.exit_codes.ERROR_SUB_PROCESS_FAILED

self.out('fermi_energy', get_fermi_energy(self.ctx.workchain.outputs['band_structure']))

self.out('bands', self.ctx.workchain.outputs['band_structure'])
return None
3 changes: 2 additions & 1 deletion aiida_common_workflows/workflows/relax/castep/generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,8 @@ def generate_inputs_base(
'kpoints_spacing': orm.Float(merged['kpoints_spacing'] / 2 / pi),
'max_iterations': orm.Int(merged['max_iterations']),
'pseudos_family': orm.Str(otfg_family.label),
'calc': calc_dictionary
'calc': calc_dictionary,
'ensure_gamma_centering': orm.Bool(merged.get('ensure_gamma_centering', False)),
}

return dictionary
Expand Down
81 changes: 81 additions & 0 deletions aiida_common_workflows/workflows/relax/castep/protocol.yml
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,84 @@ fast:
geom_force_tol: 0.05
geom_energy_tol: 2.0e-5
geom_stress_tol: 0.1


oxides_validation:
name: 'oxides-validation'
description: 'Protocol for the oxides validation study.'
relax:
relax_options:
max_meta_iterations: 5

base:
pseudos_family: 'C19'
max_iterations: 5
#kpoints_spacing: 0.04 # Equivalent to `kpoints_mp_spacing : 0.00636` - very fine...
kpoints_spacing: 0.10 # Equivalent to `kpoints_mp_spacing : 0.00636` - very fine...
ensure_gamma_centering: True # Ensure that the kpoint grid is Gamma-centered
calc:
parameters:
task: geometryoptimisation
xc_functional: pbe
symmetry_generate: true
snap_to_symmetry: true
calculate_stress: true
basis_precision: precise
write_otfg: false
opt_strategy: speed
write_bib: false
write_checkpoint: minimal
finite_basis_corr: none # No finite basis corr as we are NOT moving any atoms....
max_scf_cycles: 100
elec_energy_tol: 1e-8
geom_force_tol: 0.03
geom_energy_tol: 1.0e-5
geom_stress_tol: 0.05
grid_scale: 2
fine_grid_scale : 3
# Revised smearing scheme for the oxide validation project
smearing_width: 0.06122561905370023 # Equivalent to 0.0045 Ry
smearing_scheme: FERMIDIRAC
perc_extra_bands: 80
settings:
ADDITIONAL_RETRIEVE_TEMPORARY_LIST: ["aiida.castep_bin"]

oxides_validation_k_015:
name: 'oxides-validation-kspacing-0.15'
description: 'Protocol for the oxides validation study.'
relax:
relax_options:
max_meta_iterations: 5

base:
pseudos_family: 'C19'
max_iterations: 5
#kpoints_spacing: 0.04 # Equivalent to `kpoints_mp_spacing : 0.00636` - very fine...
kpoints_spacing: 0.15 # Increased kspacing for checks
ensure_gamma_centering: True # Ensure that the kpoint grid is Gamma-centered
calc:
parameters:
task: geometryoptimisation
xc_functional: pbe
symmetry_generate: true
snap_to_symmetry: true
calculate_stress: true
basis_precision: precise
write_otfg: false
opt_strategy: speed
write_bib: false
write_checkpoint: minimal
finite_basis_corr: none # No finite basis corr as we are NOT moving any atoms....
max_scf_cycles: 100
elec_energy_tol: 1e-8
geom_force_tol: 0.03
geom_energy_tol: 1.0e-5
geom_stress_tol: 0.05
grid_scale: 2
fine_grid_scale : 3
# Revised smearing scheme for the oxide validation project
smearing_width: 0.06122561905370023 # Equivalent to 0.0045 Ry
smearing_scheme: FERMIDIRAC
perc_extra_bands: 80
settings:
ADDITIONAL_RETRIEVE_TEMPORARY_LIST: ["aiida.castep_bin"]
1 change: 0 additions & 1 deletion aiida_common_workflows/workflows/relax/generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ def define(cls, spec):
)
spec.input(
'protocol',
valid_type=ChoiceType(('fast', 'moderate', 'precise')),
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 (

spec.input('generator_inputs.protocol', valid_type=str, non_db=True,
). So in principle I do not see a drawback in maintaining this line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point @bosonie. I should be able to just override it in CastepCommonRelaxInputGenerator. Will drop the changes to the base class.

default='moderate',
help='The protocol to use for the automated input generation. This value indicates the level of precision '
'of the results and computational cost that the input parameters will be selected for.',
Expand Down