From 8f95aa2daedf941045ae64d5e7a451840abd196a Mon Sep 17 00:00:00 2001 From: Marnik Bercx Date: Mon, 30 Aug 2021 16:11:55 +0200 Subject: [PATCH 01/11] `PwBaseWorkChain`: improve restart from `parent_folder` Remove some of the logic in the `PwBaseWorkChain` regarding restarting from a previous calculation using a `RemoteData` provided to the `pw.parent_folder` input. The current logic expected the `RemoteData` to have a `PwCalculation` creator, which is not always the case. Moreover, the `restart_mode` chosen by the user is overriden, which means that e.g. restarting from _only_ the charge density with `startingpot` is not possible. --- aiida_quantumespresso/workflows/pw/base.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/aiida_quantumespresso/workflows/pw/base.py b/aiida_quantumespresso/workflows/pw/base.py index 276b780c0..e39871ac7 100644 --- a/aiida_quantumespresso/workflows/pw/base.py +++ b/aiida_quantumespresso/workflows/pw/base.py @@ -248,9 +248,6 @@ def validate_parameters(self): self.ctx.inputs.parameters = self.ctx.inputs.parameters.get_dict() self.ctx.inputs.settings = self.ctx.inputs.settings.get_dict() if 'settings' in self.ctx.inputs else {} - if 'parent_folder' in self.ctx.inputs: - self.ctx.restart_calc = self.ctx.inputs.parent_folder.creator - self.ctx.inputs.parameters.setdefault('CONTROL', {}) self.ctx.inputs.parameters['CONTROL'].setdefault('calculation', 'scf') @@ -421,9 +418,6 @@ def prepare_process(self): if self.ctx.restart_calc: self.ctx.inputs.parameters['CONTROL']['restart_mode'] = 'restart' self.ctx.inputs.parent_folder = self.ctx.restart_calc.outputs.remote_folder - else: - self.ctx.inputs.parameters['CONTROL']['restart_mode'] = 'from_scratch' - self.ctx.inputs.pop('parent_folder', None) def report_error_handled(self, calculation, action): """Report an action taken for a calculation that has failed. From 8a0a6e98114dd6ef520144ef92853a54162e0dd2 Mon Sep 17 00:00:00 2001 From: Marnik Bercx Date: Tue, 31 Aug 2021 12:43:40 +0200 Subject: [PATCH 02/11] Add validator for the restart parameters --- aiida_quantumespresso/calculations/pw.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/aiida_quantumespresso/calculations/pw.py b/aiida_quantumespresso/calculations/pw.py index 84c9e4303..91a4a84b6 100644 --- a/aiida_quantumespresso/calculations/pw.py +++ b/aiida_quantumespresso/calculations/pw.py @@ -69,6 +69,8 @@ def define(cls, spec): help='kpoint mesh or kpoint path') spec.input('hubbard_file', valid_type=orm.SinglefileData, required=False, help='SinglefileData node containing the output Hubbard parameters from a HpCalculation') + spec.inputs.validator = cls.validate_inputs + spec.output('output_parameters', valid_type=orm.Dict, help='The `output_parameters` output node of the successful calculation.') spec.output('output_structure', valid_type=orm.StructureData, required=False, @@ -152,6 +154,26 @@ def define(cls, spec): 'is `False` and/or `electron_maxstep` is 0.') # yapf: enable + @staticmethod + def validate_inputs(value, _): + """Validate the top level namespace. + + - Check that the restart input parameters are set when a ``parent_folder`` is provided. + """ + if 'parent_folder' in value: + + parameters = value['parameters'].get_dict() + if ( + parameters.get('CONTROL', {}).get('restart_mode', None) != 'restart' and + parameters.get('ELECTRONS', {}).get('startingpot', None) != 'file' and + parameters.get('ELECTRONS', {}).get('startingwfc', None) != 'file' + ): + import warnings + warnings.warn( + '`parent_folder` input was provided for the `PwCalculation`, but no ' + 'input parameters are set to restart from these files.' + ) + @classproperty def filename_input_hubbard_parameters(cls): """Return the relative file name of the file containing the Hubbard parameters. From 7063aa2aeddd9c0731b6e8245977dd07245f3d91 Mon Sep 17 00:00:00 2001 From: Marnik Bercx Date: Tue, 31 Aug 2021 12:45:33 +0200 Subject: [PATCH 03/11] Replace `restart_calc` logic with `set_restart_type` --- aiida_quantumespresso/common/types.py | 9 +++ aiida_quantumespresso/workflows/pw/base.py | 76 +++++++++++++++------- 2 files changed, 62 insertions(+), 23 deletions(-) diff --git a/aiida_quantumespresso/common/types.py b/aiida_quantumespresso/common/types.py index ce6882b6b..db38526c9 100644 --- a/aiida_quantumespresso/common/types.py +++ b/aiida_quantumespresso/common/types.py @@ -31,3 +31,12 @@ class SpinType(enum.Enum): COLLINEAR = 'collinear' NON_COLLINEAR = 'non_collinear' SPIN_ORBIT = 'spin_orbit' + + +class RestartType(enum.Enum): + """Enumeration of ways to restart a calculation in Quantum ESPRESSO.""" + + FROM_SCRATCH = 'from_scratch' + FULL = 'full' + FROM_CHARGE_DENSITY = 'from_charge_density' + FROM_WAVE_FUNCTIONS = 'from_wave_functions' diff --git a/aiida_quantumespresso/workflows/pw/base.py b/aiida_quantumespresso/workflows/pw/base.py index e39871ac7..7a9bd071a 100644 --- a/aiida_quantumespresso/workflows/pw/base.py +++ b/aiida_quantumespresso/workflows/pw/base.py @@ -7,7 +7,7 @@ from aiida.plugins import CalculationFactory, GroupFactory from aiida_quantumespresso.calculations.functions.create_kpoints_from_distance import create_kpoints_from_distance -from aiida_quantumespresso.common.types import ElectronicType, SpinType +from aiida_quantumespresso.common.types import ElectronicType, SpinType, RestartType from aiida_quantumespresso.utils.defaults.calculation import pw as qe_defaults from aiida_quantumespresso.utils.mapping import update_mapping, prepare_process_inputs from aiida_quantumespresso.utils.pseudopotential import validate_and_prepare_pseudos_inputs @@ -236,7 +236,6 @@ def setup(self): internal loop. """ super().setup() - self.ctx.restart_calc = None self.ctx.inputs = AttributeDict(self.exposed_inputs(PwCalculation, 'pw')) def validate_parameters(self): @@ -302,6 +301,33 @@ def set_max_seconds(self, max_wallclock_seconds): max_seconds = max_wallclock_seconds * max_seconds_factor self.ctx.inputs.parameters['CONTROL']['max_seconds'] = max_seconds + def set_restart_type(self, restart_type, calculation): + """Set the restart type for the calculation.""" + + if restart_type == RestartType.FROM_SCRATCH: + self.ctx.inputs.parameters['CONTROL']['restart_mode'] = 'from_scratch' + self.ctx.inputs.parameters['ELECTRONS'].pop('startingpot', None) + self.ctx.inputs.parameters['ELECTRONS'].pop('startingwfc', None) + self.ctx.inputs.pop('parent_folder', None) + + elif restart_type == RestartType.FULL: + self.ctx.inputs.parameters['CONTROL']['restart_mode'] = 'restart' + self.ctx.inputs.parameters['ELECTRONS'].pop('startingpot', None) + self.ctx.inputs.parameters['ELECTRONS'].pop('startingwfc', None) + self.ctx.inputs.parent_folder = calculation.outputs.remote_folder + + elif restart_type == RestartType.FROM_CHARGE_DENSITY: + self.ctx.inputs.parameters['CONTROL']['restart_mode'] = 'from_scratch' + self.ctx.inputs.parameters['ELECTRONS']['startingpot'] = 'file' + self.ctx.inputs.parameters['ELECTRONS'].pop('startingwfc', None) + self.ctx.inputs.parent_folder = calculation.outputs.remote_folder + + elif restart_type == RestartType.FROM_WAVE_FUNCTIONS: + self.ctx.inputs.parameters['CONTROL']['restart_mode'] = 'from_scratch' + self.ctx.inputs.parameters['ELECTRONS'].pop('startingpot', None) + self.ctx.inputs.parameters['ELECTRONS']['startingwfc'] = 'file' + self.ctx.inputs.parent_folder = calculation.outputs.remote_folder + def should_run_init(self): """Return whether an initialization calculation should be run. @@ -404,21 +430,12 @@ def inspect_init(self): return def prepare_process(self): - """Prepare the inputs for the next calculation. - - If a `restart_calc` has been set in the context, its `remote_folder` will be used as the `parent_folder` input - for the next calculation and the `restart_mode` is set to `restart`. Otherwise, no `parent_folder` is used and - `restart_mode` is set to `from_scratch`. - """ + """Prepare the inputs for the next calculation.""" max_wallclock_seconds = self.ctx.inputs.metadata.options.get('max_wallclock_seconds', None) if max_wallclock_seconds is not None and 'max_seconds' not in self.ctx.inputs.parameters['CONTROL']: self.set_max_seconds(max_wallclock_seconds) - if self.ctx.restart_calc: - self.ctx.inputs.parameters['CONTROL']['restart_mode'] = 'restart' - self.ctx.inputs.parent_folder = self.ctx.restart_calc.outputs.remote_folder - def report_error_handled(self, calculation, action): """Report an action taken for a calculation that has failed. @@ -437,7 +454,8 @@ def sanity_check_insufficient_bands(self, calculation): Verify that the occupation of the last band is below a certain threshold, unless `occupations` was explicitly set to `fixed` in the input parameters. If this is violated, the calculation used too few bands and cannot be - trusted. The number of bands is increased and the calculation is restarted, starting from the last. + trusted. The number of bands is increased and the calculation is restarted, using the + charge density from the previous calculation. """ from aiida_quantumespresso.utils.bands import get_highest_occupied_band @@ -472,8 +490,13 @@ def sanity_check_insufficient_bands(self, calculation): nbnd_new = nbnd_cur + max(int(nbnd_cur * self.defaults.delta_factor_nbnd), self.defaults.delta_minimum_nbnd) self.ctx.inputs.parameters.setdefault('SYSTEM', {})['nbnd'] = nbnd_new + self.set_restart_type(RestartType.FROM_CHARGE_DENSITY, calculation) + + self.report( + f'Action taken: increased number of bands to {nbnd_new} and restarting from the previous charge ' + 'density.' + ) - self.report(f'Action taken: increased number of bands to {nbnd_new} and restarting from scratch') return ProcessHandlerReport(True) @process_handler(priority=600) @@ -498,14 +521,20 @@ def handle_known_unrecoverable_failure(self, calculation): PwCalculation.exit_codes.ERROR_OUT_OF_WALLTIME, ]) def handle_out_of_walltime(self, calculation): - """Handle `ERROR_OUT_OF_WALLTIME` exit code: calculation shut down neatly and we can simply restart.""" + """Handle `ERROR_OUT_OF_WALLTIME` exit code. + + In this case the calculation shut down neatly and we can simply restart. We consider two cases: + + 1. If the structure is unchanged, we do a full restart. + 2. If the structure has changed during the calculation, we restart from scratch. + """ try: self.ctx.inputs.structure = calculation.outputs.output_structure except exceptions.NotExistent: - self.ctx.restart_calc = calculation + self.set_restart_type(RestartType.FULL, calculation) self.report_error_handled(calculation, 'simply restart from the last calculation') else: - self.ctx.restart_calc = None + self.set_restart_type(RestartType.FROM_SCRATCH, calculation) self.report_error_handled(calculation, 'out of walltime: structure changed so restarting from scratch') return ProcessHandlerReport(True) @@ -521,7 +550,6 @@ def handle_vcrelax_converged_except_final_scf(self, calculation): Convergence reached in `vc-relax` except thresholds exceeded in final scf: consider as converged. """ self.ctx.is_finished = True - self.ctx.restart_calc = calculation action = 'ionic convergence thresholds met except in final scf: consider structure relaxed.' self.report_error_handled(calculation, action) self.results() # Call the results method to attach the output nodes @@ -542,7 +570,7 @@ def handle_relax_recoverable_ionic_convergence_error(self, calculation): These exit codes signify that the ionic convergence thresholds were not met, but the output structure is usable, so the solution is to simply restart from scratch but from the output structure. """ - self.ctx.restart_calc = None + self.set_restart_type(RestartType.FROM_SCRATCH, calculation) self.ctx.inputs.structure = calculation.outputs.output_structure action = 'no ionic convergence but clean shutdown: restarting from scratch but using output structure.' self.report_error_handled(calculation, action) @@ -565,7 +593,7 @@ def handle_relax_recoverable_electronic_convergence_error(self, calculation): mixing_beta = self.ctx.inputs.parameters.get('ELECTRONS', {}).get('mixing_beta', self.defaults.qe.mixing_beta) mixing_beta_new = mixing_beta * factor - self.ctx.restart_calc = None + self.set_restart_type(RestartType.FROM_SCRATCH, calculation) self.ctx.inputs.parameters.setdefault('ELECTRONS', {})['mixing_beta'] = mixing_beta_new self.ctx.inputs.structure = calculation.outputs.output_structure action = 'no electronic convergence but clean shutdown: reduced beta mixing from {} to {} restarting from ' \ @@ -577,12 +605,15 @@ def handle_relax_recoverable_electronic_convergence_error(self, calculation): PwCalculation.exit_codes.ERROR_ELECTRONIC_CONVERGENCE_NOT_REACHED, ]) def handle_electronic_convergence_not_achieved(self, calculation): - """Handle `ERROR_ELECTRONIC_CONVERGENCE_NOT_REACHED`: decrease the mixing beta and restart from scratch.""" + """Handle `ERROR_ELECTRONIC_CONVERGENCE_NOT_REACHED` error. + + Decrease the mixing beta and fully restart from the previous calculation. + """ factor = self.defaults.delta_factor_mixing_beta mixing_beta = self.ctx.inputs.parameters.get('ELECTRONS', {}).get('mixing_beta', self.defaults.qe.mixing_beta) mixing_beta_new = mixing_beta * factor - self.ctx.restart_calc = None + self.set_restart_type(RestartType.FULL, calculation) self.ctx.inputs.parameters.setdefault('ELECTRONS', {})['mixing_beta'] = mixing_beta_new action = f'reduced beta mixing from {mixing_beta} to {mixing_beta_new} and restarting from the last calculation' @@ -595,7 +626,6 @@ def handle_electronic_convergence_not_achieved(self, calculation): def handle_electronic_convergence_warning(self, calculation): """Handle `WARNING_ELECTRONIC_CONVERGENCE_NOT_REACHED': consider finished.""" self.ctx.is_finished = True - self.ctx.restart_calc = calculation action = 'electronic convergence not reached but inputs say this is ok: consider finished.' self.report_error_handled(calculation, action) self.results() # Call the results method to attach the output nodes From a9fc16409855cae99329a5d79aff59b613b1a7e4 Mon Sep 17 00:00:00 2001 From: Marnik Bercx Date: Wed, 1 Sep 2021 10:59:41 +0200 Subject: [PATCH 04/11] Apply reviewer suggestions --- aiida_quantumespresso/calculations/pw.py | 41 ++++++++++++++-------- aiida_quantumespresso/workflows/pw/base.py | 41 +++++++++++++--------- 2 files changed, 51 insertions(+), 31 deletions(-) diff --git a/aiida_quantumespresso/calculations/pw.py b/aiida_quantumespresso/calculations/pw.py index 91a4a84b6..849943693 100644 --- a/aiida_quantumespresso/calculations/pw.py +++ b/aiida_quantumespresso/calculations/pw.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- """`CalcJob` implementation for the pw.x code of Quantum ESPRESSO.""" import os +import warnings from aiida import orm from aiida.common.lang import classproperty @@ -158,21 +159,33 @@ def define(cls, spec): def validate_inputs(value, _): """Validate the top level namespace. - - Check that the restart input parameters are set when a ``parent_folder`` is provided. + 1. Check that the restart input parameters are set correctly. In case of 'nscf' and 'bands' calculations, this + means that ``parent_folder`` is provided, ``startingpot`` is set to 'file' and ``restart_mode`` is + 'from_scratch'. For other calculations, if the ``parent_folder`` is provided, the restart settings must be set + to use some of the outputs. """ - if 'parent_folder' in value: - - parameters = value['parameters'].get_dict() - if ( - parameters.get('CONTROL', {}).get('restart_mode', None) != 'restart' and - parameters.get('ELECTRONS', {}).get('startingpot', None) != 'file' and - parameters.get('ELECTRONS', {}).get('startingwfc', None) != 'file' - ): - import warnings - warnings.warn( - '`parent_folder` input was provided for the `PwCalculation`, but no ' - 'input parameters are set to restart from these files.' - ) + parameters = value['parameters'].get_dict() + calculation_type = parameters.get('CONTROL', {}).get('calculation', 'scf') + + # Check that the restart input parameters are set correctly + if calculation_type in ('nscf', 'bands'): + if 'parent_folder' not in value: + return f'`parent_folder` not provided for {calculation_type} calculation.' + if parameters.get('ELECTRONS', {}).get('startingpot', None) != 'file': + warnings.warn(f'`startingpot` should be set to `file` for a {calculation_type} calculation.') + if parameters.get('CONTROL', {}).get('CONTROL', None) == 'restart': + warnings.warn(f'`restart_mode` should be set to `from_scratch` for a {calculation_type} calculation.') + else: + if 'parent_folder' in value: + if ( + parameters.get('CONTROL', {}).get('CONTROL', None) != 'restart' and + parameters.get('ELECTRONS', {}).get('startingpot', None) != 'file' and + parameters.get('ELECTRONS', {}).get('startingwfc', None) != 'file' + ): + warnings.warn( + '`parent_folder` input was provided for the `PwCalculation`, but no ' + 'input parameters are set to restart from these files.' + ) @classproperty def filename_input_hubbard_parameters(cls): diff --git a/aiida_quantumespresso/workflows/pw/base.py b/aiida_quantumespresso/workflows/pw/base.py index 7a9bd071a..9ebc2dc93 100644 --- a/aiida_quantumespresso/workflows/pw/base.py +++ b/aiida_quantumespresso/workflows/pw/base.py @@ -248,6 +248,8 @@ def validate_parameters(self): self.ctx.inputs.settings = self.ctx.inputs.settings.get_dict() if 'settings' in self.ctx.inputs else {} self.ctx.inputs.parameters.setdefault('CONTROL', {}) + self.ctx.inputs.parameters.setdefault('ELECTRONS', {}) + self.ctx.inputs.parameters.setdefault('SYSTEM', {}) self.ctx.inputs.parameters['CONTROL'].setdefault('calculation', 'scf') def validate_kpoints(self): @@ -301,8 +303,11 @@ def set_max_seconds(self, max_wallclock_seconds): max_seconds = max_wallclock_seconds * max_seconds_factor self.ctx.inputs.parameters['CONTROL']['max_seconds'] = max_seconds - def set_restart_type(self, restart_type, calculation): - """Set the restart type for the calculation.""" + def set_restart_type(self, restart_type, parent_folder=None): + """Set the restart type for the next iteration.""" + + if parent_folder is None and restart_type != RestartType.FROM_SCRATCH: + raise ValueError('When not restarting from scratch, a `parent_folder` must be provided.') if restart_type == RestartType.FROM_SCRATCH: self.ctx.inputs.parameters['CONTROL']['restart_mode'] = 'from_scratch' @@ -314,19 +319,19 @@ def set_restart_type(self, restart_type, calculation): self.ctx.inputs.parameters['CONTROL']['restart_mode'] = 'restart' self.ctx.inputs.parameters['ELECTRONS'].pop('startingpot', None) self.ctx.inputs.parameters['ELECTRONS'].pop('startingwfc', None) - self.ctx.inputs.parent_folder = calculation.outputs.remote_folder + self.ctx.inputs.parent_folder = parent_folder elif restart_type == RestartType.FROM_CHARGE_DENSITY: self.ctx.inputs.parameters['CONTROL']['restart_mode'] = 'from_scratch' self.ctx.inputs.parameters['ELECTRONS']['startingpot'] = 'file' self.ctx.inputs.parameters['ELECTRONS'].pop('startingwfc', None) - self.ctx.inputs.parent_folder = calculation.outputs.remote_folder + self.ctx.inputs.parent_folder = parent_folder elif restart_type == RestartType.FROM_WAVE_FUNCTIONS: self.ctx.inputs.parameters['CONTROL']['restart_mode'] = 'from_scratch' self.ctx.inputs.parameters['ELECTRONS'].pop('startingpot', None) self.ctx.inputs.parameters['ELECTRONS']['startingwfc'] = 'file' - self.ctx.inputs.parent_folder = calculation.outputs.remote_folder + self.ctx.inputs.parent_folder = parent_folder def should_run_init(self): """Return whether an initialization calculation should be run. @@ -488,10 +493,9 @@ def sanity_check_insufficient_bands(self, calculation): nbnd_cur = calculation.outputs.output_parameters.get_dict()['number_of_bands'] nbnd_new = nbnd_cur + max(int(nbnd_cur * self.defaults.delta_factor_nbnd), self.defaults.delta_minimum_nbnd) + self.ctx.inputs.parameters['SYSTEM']['nbnd'] = nbnd_new - self.ctx.inputs.parameters.setdefault('SYSTEM', {})['nbnd'] = nbnd_new - self.set_restart_type(RestartType.FROM_CHARGE_DENSITY, calculation) - + self.set_restart_type(RestartType.FROM_CHARGE_DENSITY, calculation.outputs.remote_folder) self.report( f'Action taken: increased number of bands to {nbnd_new} and restarting from the previous charge ' 'density.' @@ -531,10 +535,10 @@ def handle_out_of_walltime(self, calculation): try: self.ctx.inputs.structure = calculation.outputs.output_structure except exceptions.NotExistent: - self.set_restart_type(RestartType.FULL, calculation) + self.set_restart_type(RestartType.FULL, calculation.outputs.remote_folder) self.report_error_handled(calculation, 'simply restart from the last calculation') else: - self.set_restart_type(RestartType.FROM_SCRATCH, calculation) + self.set_restart_type(RestartType.FROM_SCRATCH) self.report_error_handled(calculation, 'out of walltime: structure changed so restarting from scratch') return ProcessHandlerReport(True) @@ -570,9 +574,10 @@ def handle_relax_recoverable_ionic_convergence_error(self, calculation): These exit codes signify that the ionic convergence thresholds were not met, but the output structure is usable, so the solution is to simply restart from scratch but from the output structure. """ - self.set_restart_type(RestartType.FROM_SCRATCH, calculation) self.ctx.inputs.structure = calculation.outputs.output_structure action = 'no ionic convergence but clean shutdown: restarting from scratch but using output structure.' + + self.set_restart_type(RestartType.FROM_SCRATCH) self.report_error_handled(calculation, action) return ProcessHandlerReport(True) @@ -587,17 +592,19 @@ def handle_relax_recoverable_electronic_convergence_error(self, calculation): """Handle various exit codes for recoverable `relax` calculations with failed electronic convergence. These exit codes signify that the electronic convergence thresholds were not met, but the output structure is - usable, so the solution is to simply restart from scratch but from the output structure. + usable, so the solution is to simply restart from scratch but from the output structure and with a reduced + ``mixing_beta``. """ factor = self.defaults.delta_factor_mixing_beta mixing_beta = self.ctx.inputs.parameters.get('ELECTRONS', {}).get('mixing_beta', self.defaults.qe.mixing_beta) mixing_beta_new = mixing_beta * factor - self.set_restart_type(RestartType.FROM_SCRATCH, calculation) - self.ctx.inputs.parameters.setdefault('ELECTRONS', {})['mixing_beta'] = mixing_beta_new + self.ctx.inputs.parameters['ELECTRONS']['mixing_beta'] = mixing_beta_new self.ctx.inputs.structure = calculation.outputs.output_structure action = 'no electronic convergence but clean shutdown: reduced beta mixing from {} to {} restarting from ' \ 'scratch but using output structure.'.format(mixing_beta, mixing_beta_new) + + self.set_restart_type(RestartType.FROM_SCRATCH) self.report_error_handled(calculation, action) return ProcessHandlerReport(True) @@ -613,10 +620,10 @@ def handle_electronic_convergence_not_achieved(self, calculation): mixing_beta = self.ctx.inputs.parameters.get('ELECTRONS', {}).get('mixing_beta', self.defaults.qe.mixing_beta) mixing_beta_new = mixing_beta * factor - self.set_restart_type(RestartType.FULL, calculation) - self.ctx.inputs.parameters.setdefault('ELECTRONS', {})['mixing_beta'] = mixing_beta_new - + self.ctx.inputs.parameters['ELECTRONS']['mixing_beta'] = mixing_beta_new action = f'reduced beta mixing from {mixing_beta} to {mixing_beta_new} and restarting from the last calculation' + + self.set_restart_type(RestartType.FULL, calculation.outputs.remote_folder) self.report_error_handled(calculation, action) return ProcessHandlerReport(True) From 762edc957f8b92bfe3c4249256c3794296726ec9 Mon Sep 17 00:00:00 2001 From: Marnik Bercx Date: Fri, 10 Sep 2021 12:50:57 +0200 Subject: [PATCH 05/11] Switch to raising errors for incorrect `nscf` or `bands` restart inputs --- aiida_quantumespresso/calculations/pw.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/aiida_quantumespresso/calculations/pw.py b/aiida_quantumespresso/calculations/pw.py index 849943693..0fc62863c 100644 --- a/aiida_quantumespresso/calculations/pw.py +++ b/aiida_quantumespresso/calculations/pw.py @@ -170,18 +170,18 @@ def validate_inputs(value, _): # Check that the restart input parameters are set correctly if calculation_type in ('nscf', 'bands'): if 'parent_folder' not in value: - return f'`parent_folder` not provided for {calculation_type} calculation.' - if parameters.get('ELECTRONS', {}).get('startingpot', None) != 'file': - warnings.warn(f'`startingpot` should be set to `file` for a {calculation_type} calculation.') + return f'`parent_folder` not provided for `{calculation_type}` calculation.' + if parameters.get('ELECTRONS', {}).get('startingpot', 'file') != 'file': + return f'`startingpot` should be set to `file` for a `{calculation_type}` calculation.' if parameters.get('CONTROL', {}).get('CONTROL', None) == 'restart': - warnings.warn(f'`restart_mode` should be set to `from_scratch` for a {calculation_type} calculation.') + return f'`restart_mode` should be set to `from_scratch` for a `{calculation_type}` calculation.' else: if 'parent_folder' in value: - if ( - parameters.get('CONTROL', {}).get('CONTROL', None) != 'restart' and - parameters.get('ELECTRONS', {}).get('startingpot', None) != 'file' and - parameters.get('ELECTRONS', {}).get('startingwfc', None) != 'file' - ): + if not any([ + parameters.get('CONTROL', {}).get('CONTROL', None) == 'restart' and + parameters.get('ELECTRONS', {}).get('startingpot', None) == 'file' and + parameters.get('ELECTRONS', {}).get('startingwfc', None) == 'file' + ]): warnings.warn( '`parent_folder` input was provided for the `PwCalculation`, but no ' 'input parameters are set to restart from these files.' From a700e91d841019e9fc93cb2aacd51cb28adf4bcd Mon Sep 17 00:00:00 2001 From: Marnik Bercx Date: Tue, 21 Sep 2021 03:22:44 +0200 Subject: [PATCH 06/11] Fix PwBaseWorkChain tests --- aiida_quantumespresso/calculations/pw.py | 23 ++++--- aiida_quantumespresso/common/types.py | 2 +- aiida_quantumespresso/workflows/pw/base.py | 24 +++---- tests/conftest.py | 40 ++++++++---- tests/workflows/pw/test_base.py | 74 ++++++++++++++-------- 5 files changed, 97 insertions(+), 66 deletions(-) diff --git a/aiida_quantumespresso/calculations/pw.py b/aiida_quantumespresso/calculations/pw.py index 0fc62863c..0243f88b5 100644 --- a/aiida_quantumespresso/calculations/pw.py +++ b/aiida_quantumespresso/calculations/pw.py @@ -173,19 +173,18 @@ def validate_inputs(value, _): return f'`parent_folder` not provided for `{calculation_type}` calculation.' if parameters.get('ELECTRONS', {}).get('startingpot', 'file') != 'file': return f'`startingpot` should be set to `file` for a `{calculation_type}` calculation.' - if parameters.get('CONTROL', {}).get('CONTROL', None) == 'restart': + if parameters.get('CONTROL', {}).get('restart_mode', None) == 'restart': return f'`restart_mode` should be set to `from_scratch` for a `{calculation_type}` calculation.' - else: - if 'parent_folder' in value: - if not any([ - parameters.get('CONTROL', {}).get('CONTROL', None) == 'restart' and - parameters.get('ELECTRONS', {}).get('startingpot', None) == 'file' and - parameters.get('ELECTRONS', {}).get('startingwfc', None) == 'file' - ]): - warnings.warn( - '`parent_folder` input was provided for the `PwCalculation`, but no ' - 'input parameters are set to restart from these files.' - ) + elif 'parent_folder' in value: + if not any([ + parameters.get('CONTROL', {}).get('calculation', None) == 'restart', + parameters.get('ELECTRONS', {}).get('startingpot', None) == 'file', + parameters.get('ELECTRONS', {}).get('startingwfc', None) == 'file' + ]): + warnings.warn( + '`parent_folder` input was provided for the `PwCalculation`, but no ' + 'input parameters are set to restart from these files.' + ) @classproperty def filename_input_hubbard_parameters(cls): diff --git a/aiida_quantumespresso/common/types.py b/aiida_quantumespresso/common/types.py index db38526c9..439ed6b85 100644 --- a/aiida_quantumespresso/common/types.py +++ b/aiida_quantumespresso/common/types.py @@ -36,7 +36,7 @@ class SpinType(enum.Enum): class RestartType(enum.Enum): """Enumeration of ways to restart a calculation in Quantum ESPRESSO.""" - FROM_SCRATCH = 'from_scratch' FULL = 'full' + FROM_SCRATCH = 'from_scratch' FROM_CHARGE_DENSITY = 'from_charge_density' FROM_WAVE_FUNCTIONS = 'from_wave_functions' diff --git a/aiida_quantumespresso/workflows/pw/base.py b/aiida_quantumespresso/workflows/pw/base.py index 9ebc2dc93..3d7c89802 100644 --- a/aiida_quantumespresso/workflows/pw/base.py +++ b/aiida_quantumespresso/workflows/pw/base.py @@ -73,7 +73,6 @@ def define(cls, spec): spec.outline( cls.setup, - cls.validate_parameters, cls.validate_kpoints, cls.validate_pseudos, if_(cls.should_run_init)( @@ -230,28 +229,25 @@ def get_builder_from_protocol( return builder def setup(self): - """Call the `setup` of the `BaseRestartWorkChain` and then create the inputs dictionary in `self.ctx.inputs`. + """Call the ``setup`` of the ``BaseRestartWorkChain`` and create the inputs dictionary in ``self.ctx.inputs``. - This `self.ctx.inputs` dictionary will be used by the `BaseRestartWorkChain` to submit the calculations in the - internal loop. + This ``self.ctx.inputs`` dictionary will be used by the ``BaseRestartWorkChain`` to submit the calculations + in the internal loop. + + The ``parameters`` and ``settings`` input ``Dict`` nodes are converted into a regular dictionary and the + default namelists for the ``parameters`` are set to empty dictionaries if not specified. """ super().setup() self.ctx.inputs = AttributeDict(self.exposed_inputs(PwCalculation, 'pw')) - def validate_parameters(self): - """Validate inputs that might depend on each other and cannot be validated by the spec. - - Also define dictionary `inputs` in the context, that will contain the inputs for the calculation that will be - launched in the `run_calculation` step. - """ self.ctx.inputs.parameters = self.ctx.inputs.parameters.get_dict() - self.ctx.inputs.settings = self.ctx.inputs.settings.get_dict() if 'settings' in self.ctx.inputs else {} - self.ctx.inputs.parameters.setdefault('CONTROL', {}) self.ctx.inputs.parameters.setdefault('ELECTRONS', {}) self.ctx.inputs.parameters.setdefault('SYSTEM', {}) self.ctx.inputs.parameters['CONTROL'].setdefault('calculation', 'scf') + self.ctx.inputs.settings = self.ctx.inputs.settings.get_dict() if 'settings' in self.ctx.inputs else {} + def validate_kpoints(self): """Validate the inputs related to k-points. @@ -459,8 +455,8 @@ def sanity_check_insufficient_bands(self, calculation): Verify that the occupation of the last band is below a certain threshold, unless `occupations` was explicitly set to `fixed` in the input parameters. If this is violated, the calculation used too few bands and cannot be - trusted. The number of bands is increased and the calculation is restarted, using the - charge density from the previous calculation. + trusted. The number of bands is increased and the calculation is restarted, using the charge density from the + previous calculation. """ from aiida_quantumespresso.utils.bands import get_highest_occupied_band diff --git a/tests/conftest.py b/tests/conftest.py index d79ba007b..acf76d807 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,12 +1,13 @@ # -*- coding: utf-8 -*- # pylint: disable=redefined-outer-name,too-many-statements """Initialise a text database and profile for pytest.""" -import collections import io import os import shutil import tempfile +from collections.abc import Mapping + import pytest pytest_plugins = ['aiida.manage.tests.pytest_fixtures'] # pylint: disable=invalid-name @@ -182,7 +183,7 @@ def flatten_inputs(inputs, prefix=''): """Flatten inputs recursively like :meth:`aiida.engine.processes.process::Process._flatten_inputs`.""" flat_inputs = [] for key, value in inputs.items(): - if isinstance(value, collections.Mapping): + if isinstance(value, Mapping): flat_inputs.extend(flatten_inputs(value, prefix=prefix + key + '__')) else: flat_inputs.append((prefix + key, value)) @@ -590,10 +591,19 @@ def _generate_inputs_cp(autopilot=False): @pytest.fixture def generate_workchain_pw(generate_workchain, generate_inputs_pw, generate_calc_job_node): - """Generate an instance of a `PwBaseWorkChain`.""" + """Generate an instance of a ``PwBaseWorkChain``.""" + + def _generate_workchain_pw(exit_code=None, inputs=None, return_inputs=False, pw_outputs=None): + """Generate an instance of a ``PwBaseWorkChain``. - def _generate_workchain_pw(exit_code=None, inputs=None, return_inputs=False): + :param exit_code: exit code for the ``PwCalculation``. + :param inputs: inputs for the ``PwBaseWorkChain``. + :param return_inputs: return the inputs of the ``PwBaseWorkChain``. + :param pw_outputs: ``dict`` of outputs for the ``PwCalculation``. The keys must correspond to the link labels + and the values to the output nodes. + """ from plumpy import ProcessState + from aiida.common import LinkType from aiida.orm import Dict entry_point = 'quantumespresso.pw.base' @@ -608,13 +618,18 @@ def _generate_workchain_pw(exit_code=None, inputs=None, return_inputs=False): process = generate_workchain(entry_point, inputs) - if exit_code is not None: - node = generate_calc_job_node(inputs={'parameters': Dict()}) - node.set_process_state(ProcessState.FINISHED) - node.set_exit_status(exit_code.status) + pw_node = generate_calc_job_node(inputs={'parameters': Dict()}) + process.ctx.iteration = 1 + process.ctx.children = [pw_node] - process.ctx.iteration = 1 - process.ctx.children = [node] + if pw_outputs is not None: + for link_label, output_node in pw_outputs.items(): + output_node.add_incoming(pw_node, link_type=LinkType.CREATE, link_label=link_label) + output_node.store() + + if exit_code is not None: + pw_node.set_process_state(ProcessState.FINISHED) + pw_node.set_exit_status(exit_code.status) return process @@ -652,7 +667,9 @@ def _generate_workchain_ph(exit_code=None, inputs=None, return_inputs=False): @pytest.fixture -def generate_workchain_pdos(generate_workchain, generate_inputs_pw, fixture_code): +def generate_workchain_pdos( + generate_workchain, generate_inputs_pw, fixture_localhost, fixture_code, generate_remote_data +): """Generate an instance of a `PdosWorkChain`.""" def _generate_workchain_pdos(): @@ -672,6 +689,7 @@ def _generate_workchain_pdos(): nscf_pw_inputs['parameters']['CONTROL']['calculation'] = 'nscf' nscf_pw_inputs['parameters']['SYSTEM']['occupations'] = 'tetrahedra' nscf_pw_inputs['parameters']['SYSTEM']['nosym'] = True + nscf_pw_inputs['parent_folder'] = generate_remote_data(computer=fixture_localhost, remote_path='/daddy_is_here') nscf = {'pw': nscf_pw_inputs, 'kpoints': kpoints} diff --git a/tests/workflows/pw/test_base.py b/tests/workflows/pw/test_base.py index 8c915889a..d7133d4ed 100644 --- a/tests/workflows/pw/test_base.py +++ b/tests/workflows/pw/test_base.py @@ -15,7 +15,6 @@ def test_setup(generate_workchain_pw): process = generate_workchain_pw() process.setup() - assert process.ctx.restart_calc is None assert isinstance(process.ctx.inputs, AttributeDict) @@ -40,29 +39,59 @@ def test_handle_unrecoverable_failure(generate_workchain_pw): assert result == PwBaseWorkChain.exit_codes.ERROR_UNRECOVERABLE_FAILURE -def test_handle_out_of_walltime(generate_workchain_pw): +def test_handle_out_of_walltime(generate_workchain_pw, fixture_localhost, generate_remote_data): """Test `PwBaseWorkChain.handle_out_of_walltime`.""" - process = generate_workchain_pw(exit_code=PwCalculation.exit_codes.ERROR_OUT_OF_WALLTIME) + remote_data = generate_remote_data(computer=fixture_localhost, remote_path='/come_to_papa') + process = generate_workchain_pw( + exit_code=PwCalculation.exit_codes.ERROR_OUT_OF_WALLTIME, pw_outputs={'remote_folder': remote_data} + ) process.setup() + result = process.handle_electronic_convergence_not_achieved(process.ctx.children[-1]) result = process.handle_out_of_walltime(process.ctx.children[-1]) assert isinstance(result, ProcessHandlerReport) + assert process.ctx.inputs.parameters['CONTROL']['restart_mode'] == 'restart' assert result.do_break result = process.inspect_process() assert result.status == 0 -def test_handle_electronic_convergence_not_reached(generate_workchain_pw): +def test_handle_out_of_walltime_structure_changed(generate_workchain_pw, generate_structure): + """Test `PwBaseWorkChain.handle_out_of_walltime`.""" + structure = generate_structure() + process = generate_workchain_pw( + exit_code=PwCalculation.exit_codes.ERROR_OUT_OF_WALLTIME, pw_outputs={'output_structure': structure} + ) + process.setup() + + result = process.handle_out_of_walltime(process.ctx.children[-1]) + assert isinstance(result, ProcessHandlerReport) + assert process.ctx.inputs.parameters['CONTROL']['restart_mode'] == 'from_scratch' + assert result.do_break + + result = process.inspect_process() + assert result.status == 0 + + +def test_handle_electronic_convergence_not_achieved(generate_workchain_pw, fixture_localhost, generate_remote_data): """Test `PwBaseWorkChain.handle_electronic_convergence_not_achieved`.""" - process = generate_workchain_pw(exit_code=PwCalculation.exit_codes.ERROR_ELECTRONIC_CONVERGENCE_NOT_REACHED) + remote_data = generate_remote_data(computer=fixture_localhost, remote_path='/come_to_papa') + + process = generate_workchain_pw( + exit_code=PwCalculation.exit_codes.ERROR_ELECTRONIC_CONVERGENCE_NOT_REACHED, + pw_outputs={'remote_folder': remote_data} + ) process.setup() - process.validate_parameters() + + process.ctx.inputs.parameters['ELECTRONS']['mixing_beta'] = 0.5 result = process.handle_electronic_convergence_not_achieved(process.ctx.children[-1]) assert isinstance(result, ProcessHandlerReport) + assert process.ctx.inputs.parameters['ELECTRONS']['mixing_beta'] == \ + process.defaults.delta_factor_mixing_beta * 0.5 + assert process.ctx.inputs.parameters['CONTROL']['restart_mode'] == 'restart' assert result.do_break - assert process.ctx.restart_calc is None result = process.inspect_process() assert result.status == 0 @@ -95,7 +124,6 @@ def test_handle_vcrelax_converged_except_final_scf(generate_workchain_pw): assert result.do_break assert result.exit_code == PwBaseWorkChain.exit_codes.ERROR_IONIC_CONVERGENCE_REACHED_EXCEPT_IN_FINAL_SCF assert process.node.get_outgoing().all() is not None - assert process.ctx.restart_calc is calculation result = process.inspect_process() assert result == PwBaseWorkChain.exit_codes.ERROR_IONIC_CONVERGENCE_REACHED_EXCEPT_IN_FINAL_SCF @@ -111,20 +139,15 @@ def test_handle_vcrelax_converged_except_final_scf(generate_workchain_pw): ) def test_handle_relax_recoverable_ionic_convergence_error(generate_workchain_pw, generate_structure, exit_code): """Test `PwBaseWorkChain.handle_relax_recoverable_ionic_convergence_error`.""" - from aiida.common.links import LinkType - - process = generate_workchain_pw(exit_code=exit_code) + structure = generate_structure() + process = generate_workchain_pw(pw_outputs={'output_structure': structure}, exit_code=exit_code) process.setup() - calculation = process.ctx.children[-1] - structure = generate_structure().store() - structure.add_incoming(calculation, link_label='output_structure', link_type=LinkType.CREATE) - - result = process.handle_relax_recoverable_ionic_convergence_error(calculation) + result = process.handle_relax_recoverable_ionic_convergence_error(process.ctx.children[-1]) assert isinstance(result, ProcessHandlerReport) assert result.do_break assert result.exit_code.status == 0 - assert process.ctx.restart_calc is None + assert process.ctx.inputs.parameters['CONTROL']['restart_mode'] == 'from_scratch' result = process.inspect_process() assert result.status == 0 @@ -132,27 +155,24 @@ def test_handle_relax_recoverable_ionic_convergence_error(generate_workchain_pw, def test_handle_electronic_convergence_warning(generate_workchain_pw, generate_structure): """Test `PwBaseWorkChain.handle_electronic_convergence_warning`.""" - from aiida.common.links import LinkType - inputs = generate_workchain_pw(return_inputs=True) inputs['pw']['parameters']['scf_maxstep'] = 0 inputs['pw']['parameters']['scf_must_converge'] = False + structure = generate_structure() + process = generate_workchain_pw( - exit_code=PwBaseWorkChain.exit_codes.WARNING_ELECTRONIC_CONVERGENCE_NOT_REACHED, inputs=inputs + exit_code=PwBaseWorkChain.exit_codes.WARNING_ELECTRONIC_CONVERGENCE_NOT_REACHED, + inputs=inputs, + pw_outputs={'output_structure': structure} ) process.setup() - calculation = process.ctx.children[-1] - structure = generate_structure().store() - structure.add_incoming(calculation, link_label='output_structure', link_type=LinkType.CREATE) - - result = process.handle_electronic_convergence_warning(calculation) + result = process.handle_electronic_convergence_warning(process.ctx.children[-1]) assert isinstance(result, ProcessHandlerReport) assert result.do_break assert result.exit_code == PwBaseWorkChain.exit_codes.WARNING_ELECTRONIC_CONVERGENCE_NOT_REACHED assert process.node.get_outgoing().all() is not None - assert process.ctx.restart_calc is calculation result = process.inspect_process() assert result == PwBaseWorkChain.exit_codes.WARNING_ELECTRONIC_CONVERGENCE_NOT_REACHED @@ -174,7 +194,6 @@ def test_set_max_seconds(generate_workchain_pw): process = generate_workchain_pw(inputs=inputs) process.setup() - process.validate_parameters() process.prepare_process() expected_max_seconds = max_wallclock_seconds * process.defaults.delta_factor_max_seconds @@ -189,7 +208,6 @@ def test_set_max_seconds(generate_workchain_pw): process = generate_workchain_pw(inputs=inputs) process.setup() - process.validate_parameters() process.prepare_process() assert 'max_seconds' in process.ctx.inputs['parameters']['CONTROL'] From ca2ec288d2b81dc28863d9c697ca4ea1b2477103 Mon Sep 17 00:00:00 2001 From: Marnik Bercx Date: Tue, 21 Sep 2021 04:38:21 +0200 Subject: [PATCH 07/11] Add tests for restart validation --- aiida_quantumespresso/calculations/pw.py | 2 +- tests/calculations/test_pw.py | 64 +++++++++++++++++++ tests/calculations/test_pw/test_pw_default.in | 1 + tests/conftest.py | 31 +++++---- tests/tools/test_immigrate.py | 3 + 5 files changed, 89 insertions(+), 12 deletions(-) diff --git a/aiida_quantumespresso/calculations/pw.py b/aiida_quantumespresso/calculations/pw.py index 0243f88b5..842dcbc3b 100644 --- a/aiida_quantumespresso/calculations/pw.py +++ b/aiida_quantumespresso/calculations/pw.py @@ -177,7 +177,7 @@ def validate_inputs(value, _): return f'`restart_mode` should be set to `from_scratch` for a `{calculation_type}` calculation.' elif 'parent_folder' in value: if not any([ - parameters.get('CONTROL', {}).get('calculation', None) == 'restart', + parameters.get('CONTROL', {}).get('restart_mode', None) == 'restart', parameters.get('ELECTRONS', {}).get('startingpot', None) == 'file', parameters.get('ELECTRONS', {}).get('startingwfc', None) == 'file' ]): diff --git a/tests/calculations/test_pw.py b/tests/calculations/test_pw.py index de2a65f98..45c63b247 100644 --- a/tests/calculations/test_pw.py +++ b/tests/calculations/test_pw.py @@ -250,3 +250,67 @@ def test_pw_parallelization_duplicate_cmdline_flag(fixture_sandbox, generate_cal with pytest.raises(InputValidationError) as exc: generate_calc_job(fixture_sandbox, entry_point_name, inputs) assert 'Conflicting' in str(exc.value) + + +@pytest.mark.parametrize('calculation', ['nscf', 'bands', 'scf', 'relax', 'vc-relax']) +def test_pw_validate_inputs_restart( + fixture_sandbox, generate_calc_job, generate_inputs_pw, fixture_localhost, generate_remote_data, calculation +): + """Test the input validation of restart settings for the ``PwCalculation``.""" + remote_data = generate_remote_data(computer=fixture_localhost, remote_path='/cat_stevens') + entry_point_name = 'quantumespresso.pw' + + inputs = generate_inputs_pw() + parameters = inputs['parameters'].get_dict() + parameters['CONTROL']['calculation'] = calculation + + if calculation in ('nscf', 'bands'): + + # No parent_folder -> raise + inputs['parameters'] = orm.Dict(dict=parameters) + with pytest.raises(ValueError) as exc: + generate_calc_job(fixture_sandbox, entry_point_name, inputs) + assert f'`parent_folder` not provided for `{calculation}` calculation.' in str(exc.value) + + # Parent_folder + defaults -> works + inputs['parent_folder'] = remote_data + generate_calc_job(fixture_sandbox, entry_point_name, inputs) + + # Set `startingpot` to `'atomic'` -> raise + parameters['ELECTRONS']['startingpot'] = 'atomic' + inputs['parameters'] = orm.Dict(dict=parameters) + with pytest.raises(ValueError) as exc: + generate_calc_job(fixture_sandbox, entry_point_name, inputs) + assert f'`startingpot` should be set to `file` for a `{calculation}` calculation.' in str(exc.value) + + # Set `restart_mode` to `'restart'` -> raise + parameters['ELECTRONS'].pop('startingpot') + parameters['CONTROL']['restart_mode'] = 'restart' + inputs['parameters'] = orm.Dict(dict=parameters) + with pytest.raises(ValueError) as exc: + generate_calc_job(fixture_sandbox, entry_point_name, inputs) + assert f'`restart_mode` should be set to `from_scratch` for a `{calculation}` calculation.' in str(exc.value) + + else: + # Add `parent_folder` but no restart tags -> warning + inputs['parent_folder'] = remote_data + with pytest.warns(Warning) as warnings: + generate_calc_job(fixture_sandbox, entry_point_name, inputs) + assert any('`parent_folder` input was provided for the' in str(warning.message) for warning in warnings.list) + + # Set `restart_mode` to `'restart'` -> no warning + parameters['CONTROL']['restart_mode'] = 'restart' + inputs['parameters'] = orm.Dict(dict=parameters) + with pytest.warns(None) as warnings: + generate_calc_job(fixture_sandbox, entry_point_name, inputs) + assert len(warnings.list) == 0 + parameters['CONTROL'].pop('restart_mode') + + # Set `startingwfc` or `startingpot` to `'file'` -> no warning + for restart_setting in ('startingpot', 'startingwfc'): + parameters['ELECTRONS'][restart_setting] = 'file' + inputs['parameters'] = orm.Dict(dict=parameters) + with pytest.warns(None) as warnings: + generate_calc_job(fixture_sandbox, entry_point_name, inputs) + assert len(warnings.list) == 0 + parameters['ELECTRONS'].pop(restart_setting) diff --git a/tests/calculations/test_pw/test_pw_default.in b/tests/calculations/test_pw/test_pw_default.in index ef7e9dd07..c9d3e47d1 100644 --- a/tests/calculations/test_pw/test_pw_default.in +++ b/tests/calculations/test_pw/test_pw_default.in @@ -13,6 +13,7 @@ ntyp = 1 / &ELECTRONS + electron_maxstep = 60 / ATOMIC_SPECIES Si 28.0855 Si.upf diff --git a/tests/conftest.py b/tests/conftest.py index acf76d807..fc2d058de 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -516,18 +516,27 @@ def _generate_inputs_pw(): from aiida_quantumespresso.utils.resources import get_default_options inputs = { - 'code': fixture_code('quantumespresso.pw'), - 'structure': generate_structure(), - 'kpoints': generate_kpoints_mesh(2), - 'parameters': Dict(dict={ - 'CONTROL': { - 'calculation': 'scf' - }, - 'SYSTEM': { - 'ecutrho': 240.0, - 'ecutwfc': 30.0 + 'code': + fixture_code('quantumespresso.pw'), + 'structure': + generate_structure(), + 'kpoints': + generate_kpoints_mesh(2), + 'parameters': + Dict( + dict={ + 'CONTROL': { + 'calculation': 'scf' + }, + 'SYSTEM': { + 'ecutrho': 240.0, + 'ecutwfc': 30.0 + }, + 'ELECTRONS': { + 'electron_maxstep': 60, + } } - }), + ), 'pseudos': { 'Si': generate_upf_data('Si') }, diff --git a/tests/tools/test_immigrate.py b/tests/tools/test_immigrate.py index ad38c26d0..c205fe706 100644 --- a/tests/tools/test_immigrate.py +++ b/tests/tools/test_immigrate.py @@ -52,6 +52,9 @@ def test_create_builder(fixture_sandbox, fixture_code, generate_upf_data, genera 'ecutrho': 240.0, 'ecutwfc': 30.0, 'ibrav': 0, + }, + 'ELECTRONS': { + 'electron_maxstep': 60 } } assert 'kpoints' in builder From c7b897c1302f950e9b61b2658b78893b6e937b75 Mon Sep 17 00:00:00 2001 From: Marnik Bercx Date: Tue, 21 Sep 2021 10:34:15 +0200 Subject: [PATCH 08/11] apply excellent reviewer suggestions --- aiida_quantumespresso/calculations/pw.py | 2 +- tests/conftest.py | 40 +++++++++++------------- 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/aiida_quantumespresso/calculations/pw.py b/aiida_quantumespresso/calculations/pw.py index 842dcbc3b..cc9dfc61e 100644 --- a/aiida_quantumespresso/calculations/pw.py +++ b/aiida_quantumespresso/calculations/pw.py @@ -173,7 +173,7 @@ def validate_inputs(value, _): return f'`parent_folder` not provided for `{calculation_type}` calculation.' if parameters.get('ELECTRONS', {}).get('startingpot', 'file') != 'file': return f'`startingpot` should be set to `file` for a `{calculation_type}` calculation.' - if parameters.get('CONTROL', {}).get('restart_mode', None) == 'restart': + if parameters.get('CONTROL', {}).get('restart_mode', 'from_scratch') != 'from_scratch': return f'`restart_mode` should be set to `from_scratch` for a `{calculation_type}` calculation.' elif 'parent_folder' in value: if not any([ diff --git a/tests/conftest.py b/tests/conftest.py index fc2d058de..8cc0c7056 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -515,28 +515,25 @@ def _generate_inputs_pw(): from aiida.orm import Dict from aiida_quantumespresso.utils.resources import get_default_options - inputs = { - 'code': - fixture_code('quantumespresso.pw'), - 'structure': - generate_structure(), - 'kpoints': - generate_kpoints_mesh(2), - 'parameters': - Dict( - dict={ - 'CONTROL': { - 'calculation': 'scf' - }, - 'SYSTEM': { - 'ecutrho': 240.0, - 'ecutwfc': 30.0 - }, - 'ELECTRONS': { - 'electron_maxstep': 60, - } + parameters = Dict( + dict={ + 'CONTROL': { + 'calculation': 'scf' + }, + 'SYSTEM': { + 'ecutrho': 240.0, + 'ecutwfc': 30.0 + }, + 'ELECTRONS': { + 'electron_maxstep': 60, } - ), + } + ) + inputs = { + 'code': fixture_code('quantumespresso.pw'), + 'structure': generate_structure(), + 'kpoints': generate_kpoints_mesh(2), + 'parameters': parameters, 'pseudos': { 'Si': generate_upf_data('Si') }, @@ -544,7 +541,6 @@ def _generate_inputs_pw(): 'options': get_default_options() } } - return inputs return _generate_inputs_pw From d4eedce646bcc1cb186f4363f238d18a3e7fde4b Mon Sep 17 00:00:00 2001 From: Marnik Bercx Date: Tue, 21 Sep 2021 13:10:13 +0200 Subject: [PATCH 09/11] clean up the tests a bit from my silly comments --- tests/calculations/test_pw.py | 96 +++++++++++++++++---------------- tests/conftest.py | 4 +- tests/workflows/pw/test_base.py | 4 +- 3 files changed, 56 insertions(+), 48 deletions(-) diff --git a/tests/calculations/test_pw.py b/tests/calculations/test_pw.py index 45c63b247..db70ffb30 100644 --- a/tests/calculations/test_pw.py +++ b/tests/calculations/test_pw.py @@ -252,65 +252,71 @@ def test_pw_parallelization_duplicate_cmdline_flag(fixture_sandbox, generate_cal assert 'Conflicting' in str(exc.value) -@pytest.mark.parametrize('calculation', ['nscf', 'bands', 'scf', 'relax', 'vc-relax']) -def test_pw_validate_inputs_restart( +@pytest.mark.parametrize('calculation', ['scf', 'relax', 'vc-relax']) +def test_pw_validate_inputs_restart_base( fixture_sandbox, generate_calc_job, generate_inputs_pw, fixture_localhost, generate_remote_data, calculation ): """Test the input validation of restart settings for the ``PwCalculation``.""" - remote_data = generate_remote_data(computer=fixture_localhost, remote_path='/cat_stevens') + remote_data = generate_remote_data(computer=fixture_localhost, remote_path='/path/to/remote') entry_point_name = 'quantumespresso.pw' inputs = generate_inputs_pw() parameters = inputs['parameters'].get_dict() parameters['CONTROL']['calculation'] = calculation - if calculation in ('nscf', 'bands'): - - # No parent_folder -> raise - inputs['parameters'] = orm.Dict(dict=parameters) - with pytest.raises(ValueError) as exc: - generate_calc_job(fixture_sandbox, entry_point_name, inputs) - assert f'`parent_folder` not provided for `{calculation}` calculation.' in str(exc.value) + # Add `parent_folder` but no restart tags -> warning + inputs['parent_folder'] = remote_data + with pytest.warns(Warning, match='`parent_folder` input was provided for the'): + generate_calc_job(fixture_sandbox, entry_point_name, inputs) - # Parent_folder + defaults -> works - inputs['parent_folder'] = remote_data + # Set `restart_mode` to `'restart'` -> no warning + parameters['CONTROL']['restart_mode'] = 'restart' + inputs['parameters'] = orm.Dict(dict=parameters) + with pytest.warns(None) as warnings: generate_calc_job(fixture_sandbox, entry_point_name, inputs) + assert len(warnings.list) == 0 + parameters['CONTROL'].pop('restart_mode') - # Set `startingpot` to `'atomic'` -> raise - parameters['ELECTRONS']['startingpot'] = 'atomic' + # Set `startingwfc` or `startingpot` to `'file'` -> no warning + for restart_setting in ('startingpot', 'startingwfc'): + parameters['ELECTRONS'][restart_setting] = 'file' inputs['parameters'] = orm.Dict(dict=parameters) - with pytest.raises(ValueError) as exc: + with pytest.warns(None) as warnings: generate_calc_job(fixture_sandbox, entry_point_name, inputs) - assert f'`startingpot` should be set to `file` for a `{calculation}` calculation.' in str(exc.value) + assert len(warnings.list) == 0 + parameters['ELECTRONS'].pop(restart_setting) - # Set `restart_mode` to `'restart'` -> raise - parameters['ELECTRONS'].pop('startingpot') - parameters['CONTROL']['restart_mode'] = 'restart' - inputs['parameters'] = orm.Dict(dict=parameters) - with pytest.raises(ValueError) as exc: - generate_calc_job(fixture_sandbox, entry_point_name, inputs) - assert f'`restart_mode` should be set to `from_scratch` for a `{calculation}` calculation.' in str(exc.value) - else: - # Add `parent_folder` but no restart tags -> warning - inputs['parent_folder'] = remote_data - with pytest.warns(Warning) as warnings: - generate_calc_job(fixture_sandbox, entry_point_name, inputs) - assert any('`parent_folder` input was provided for the' in str(warning.message) for warning in warnings.list) +@pytest.mark.parametrize('calculation', ['nscf', 'bands']) +def test_pw_validate_inputs_restart_nscf( + fixture_sandbox, generate_calc_job, generate_inputs_pw, fixture_localhost, generate_remote_data, calculation +): + """Test the input validation of restart settings for the ``PwCalculation``.""" + remote_data = generate_remote_data(computer=fixture_localhost, remote_path='/path/to/remote') + entry_point_name = 'quantumespresso.pw' - # Set `restart_mode` to `'restart'` -> no warning - parameters['CONTROL']['restart_mode'] = 'restart' - inputs['parameters'] = orm.Dict(dict=parameters) - with pytest.warns(None) as warnings: - generate_calc_job(fixture_sandbox, entry_point_name, inputs) - assert len(warnings.list) == 0 - parameters['CONTROL'].pop('restart_mode') - - # Set `startingwfc` or `startingpot` to `'file'` -> no warning - for restart_setting in ('startingpot', 'startingwfc'): - parameters['ELECTRONS'][restart_setting] = 'file' - inputs['parameters'] = orm.Dict(dict=parameters) - with pytest.warns(None) as warnings: - generate_calc_job(fixture_sandbox, entry_point_name, inputs) - assert len(warnings.list) == 0 - parameters['ELECTRONS'].pop(restart_setting) + inputs = generate_inputs_pw() + parameters = inputs['parameters'].get_dict() + parameters['CONTROL']['calculation'] = calculation + + # No parent_folder -> raise + inputs['parameters'] = orm.Dict(dict=parameters) + with pytest.raises(ValueError, match='`parent_folder` not provided for `.*` calculation.'): + generate_calc_job(fixture_sandbox, entry_point_name, inputs) + + # Parent_folder + defaults -> works + inputs['parent_folder'] = remote_data + generate_calc_job(fixture_sandbox, entry_point_name, inputs) + + # Set `startingpot` to `'atomic'` -> raise + parameters['ELECTRONS']['startingpot'] = 'atomic' + inputs['parameters'] = orm.Dict(dict=parameters) + with pytest.raises(ValueError, match='`startingpot` should be set to `file` for a `.*` calculation.'): + generate_calc_job(fixture_sandbox, entry_point_name, inputs) + + # Set `restart_mode` to `'restart'` -> raise + parameters['ELECTRONS'].pop('startingpot') + parameters['CONTROL']['restart_mode'] = 'restart' + inputs['parameters'] = orm.Dict(dict=parameters) + with pytest.raises(ValueError, match='`restart_mode` should be set to `from_scratch` for a `.*`.'): + generate_calc_job(fixture_sandbox, entry_point_name, inputs) diff --git a/tests/conftest.py b/tests/conftest.py index 8cc0c7056..2f106d486 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -694,7 +694,9 @@ def _generate_workchain_pdos(): nscf_pw_inputs['parameters']['CONTROL']['calculation'] = 'nscf' nscf_pw_inputs['parameters']['SYSTEM']['occupations'] = 'tetrahedra' nscf_pw_inputs['parameters']['SYSTEM']['nosym'] = True - nscf_pw_inputs['parent_folder'] = generate_remote_data(computer=fixture_localhost, remote_path='/daddy_is_here') + nscf_pw_inputs['parent_folder'] = generate_remote_data( + computer=fixture_localhost, remote_path='/path/to/remote' + ) nscf = {'pw': nscf_pw_inputs, 'kpoints': kpoints} diff --git a/tests/workflows/pw/test_base.py b/tests/workflows/pw/test_base.py index d7133d4ed..d99b783fb 100644 --- a/tests/workflows/pw/test_base.py +++ b/tests/workflows/pw/test_base.py @@ -41,7 +41,7 @@ def test_handle_unrecoverable_failure(generate_workchain_pw): def test_handle_out_of_walltime(generate_workchain_pw, fixture_localhost, generate_remote_data): """Test `PwBaseWorkChain.handle_out_of_walltime`.""" - remote_data = generate_remote_data(computer=fixture_localhost, remote_path='/come_to_papa') + remote_data = generate_remote_data(computer=fixture_localhost, remote_path='/path/to/remote') process = generate_workchain_pw( exit_code=PwCalculation.exit_codes.ERROR_OUT_OF_WALLTIME, pw_outputs={'remote_folder': remote_data} ) @@ -76,7 +76,7 @@ def test_handle_out_of_walltime_structure_changed(generate_workchain_pw, generat def test_handle_electronic_convergence_not_achieved(generate_workchain_pw, fixture_localhost, generate_remote_data): """Test `PwBaseWorkChain.handle_electronic_convergence_not_achieved`.""" - remote_data = generate_remote_data(computer=fixture_localhost, remote_path='/come_to_papa') + remote_data = generate_remote_data(computer=fixture_localhost, remote_path='/path/to/remote') process = generate_workchain_pw( exit_code=PwCalculation.exit_codes.ERROR_ELECTRONIC_CONVERGENCE_NOT_REACHED, From 3aa820cdb0d478bd6299601ef9f7f2cd4cea12a4 Mon Sep 17 00:00:00 2001 From: Marnik Bercx Date: Tue, 21 Sep 2021 14:36:09 +0200 Subject: [PATCH 10/11] Raise warning instead of error for nscf/bands and `restart_mode==from_scratch` --- aiida_quantumespresso/calculations/pw.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aiida_quantumespresso/calculations/pw.py b/aiida_quantumespresso/calculations/pw.py index cc9dfc61e..40be66aa3 100644 --- a/aiida_quantumespresso/calculations/pw.py +++ b/aiida_quantumespresso/calculations/pw.py @@ -174,7 +174,7 @@ def validate_inputs(value, _): if parameters.get('ELECTRONS', {}).get('startingpot', 'file') != 'file': return f'`startingpot` should be set to `file` for a `{calculation_type}` calculation.' if parameters.get('CONTROL', {}).get('restart_mode', 'from_scratch') != 'from_scratch': - return f'`restart_mode` should be set to `from_scratch` for a `{calculation_type}` calculation.' + warnings.warn(f'`restart_mode` should be set to `from_scratch` for a `{calculation_type}` calculation.') elif 'parent_folder' in value: if not any([ parameters.get('CONTROL', {}).get('restart_mode', None) == 'restart', From 2c08469d424d0e94564eaa093d30e60c5146ddbe Mon Sep 17 00:00:00 2001 From: Marnik Bercx Date: Tue, 21 Sep 2021 14:47:09 +0200 Subject: [PATCH 11/11] And then fix the test, of course --- tests/calculations/test_pw.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/calculations/test_pw.py b/tests/calculations/test_pw.py index db70ffb30..65989407f 100644 --- a/tests/calculations/test_pw.py +++ b/tests/calculations/test_pw.py @@ -314,9 +314,9 @@ def test_pw_validate_inputs_restart_nscf( with pytest.raises(ValueError, match='`startingpot` should be set to `file` for a `.*` calculation.'): generate_calc_job(fixture_sandbox, entry_point_name, inputs) - # Set `restart_mode` to `'restart'` -> raise + # Set `restart_mode` to `'restart'` -> warning parameters['ELECTRONS'].pop('startingpot') parameters['CONTROL']['restart_mode'] = 'restart' inputs['parameters'] = orm.Dict(dict=parameters) - with pytest.raises(ValueError, match='`restart_mode` should be set to `from_scratch` for a `.*`.'): + with pytest.warns(Warning, match='`restart_mode` should be set to `from_scratch` for a `.*`.'): generate_calc_job(fixture_sandbox, entry_point_name, inputs)