From e5b70064b624f353a716e9c485c3281c7111a8f4 Mon Sep 17 00:00:00 2001 From: "Jose M. Pizarro" <112697669+JosePizarro3@users.noreply.github.com> Date: Wed, 3 Apr 2024 10:44:52 +0200 Subject: [PATCH] General minor fixes after meeting (#37) * Fixes in outputs.py and testing * Improved SelfConsistency --- src/nomad_simulations/numerical_settings.py | 29 ++++------ src/nomad_simulations/outputs.py | 63 ++++----------------- tests/test_outputs.py | 48 ++++------------ 3 files changed, 30 insertions(+), 110 deletions(-) diff --git a/src/nomad_simulations/numerical_settings.py b/src/nomad_simulations/numerical_settings.py index b3999bb4..c0781d55 100644 --- a/src/nomad_simulations/numerical_settings.py +++ b/src/nomad_simulations/numerical_settings.py @@ -444,17 +444,12 @@ def normalize(self, archive, logger) -> None: class SelfConsistency(NumericalSettings): """ A base section used to define the convergence settings of self-consistent field (SCF) calculation. - It determines the condictions for `is_converged` in properties `Outputs` (see outputs.py). The convergence + It determines the condictions for `is_scf_converged` in `SCFOutputs` (see outputs.py). The convergence criteria covered are: 1. The number of iterations is smaller than or equal to `n_max_iterations`. - - and one of the following: - - 2a. The total energy change between two subsequent self-consistent iterations is below - `threshold_energy_change`. - 2b. The charge density change between two subsequent self-consistent iterations is below - `threshold_charge_density_change`. + 2. The total change between two subsequent self-consistent iterations for an output property is below + `threshold_change`. """ # TODO add examples or MEnum? @@ -468,28 +463,24 @@ class SelfConsistency(NumericalSettings): n_max_iterations = Quantity( type=np.int32, description=""" - Specifies the maximum number of allowed self-consistent iterations. The simulation `is_converged` + Specifies the maximum number of allowed self-consistent iterations. The simulation `is_scf_converged` if the number of iterations is not larger or equal than this quantity. """, ) - # ? define class `Tolerance` for the different Scf tolerances types? - threshold_energy_change = Quantity( + threshold_change = Quantity( type=np.float64, - unit='joule', description=""" - Specifies the threshold for the total energy change between two subsequent self-consistent iterations. - The simulation `is_converged` if the total energy change between two SCF cycles is below + Specifies the threshold for the change between two subsequent self-consistent iterations on + a given output property. The simulation `is_scf_converged` if this total change is below this threshold. """, ) - threshold_charge_density_change = Quantity( - type=np.float64, + threshold_change_unit = Quantity( + type=str, description=""" - Specifies the threshold for the average charge density change between two subsequent - self-consistent iterations. The simulation `is_converged` if the charge density change - between two SCF cycles is below this threshold. + Unit using the pint UnitRegistry() notation for the `threshold_change`. """, ) diff --git a/src/nomad_simulations/outputs.py b/src/nomad_simulations/outputs.py index 3667b968..cc42673a 100644 --- a/src/nomad_simulations/outputs.py +++ b/src/nomad_simulations/outputs.py @@ -99,37 +99,25 @@ class Outputs(ArchiveSection): a_eln=ELNAnnotation(component='ReferenceEditQuantity'), ) - def check_is_derived(self, is_derived: bool, outputs_ref) -> Optional[bool]: + def resolve_is_derived(self, outputs_ref) -> bool: """ - Check if the output property is derived or not. + Resolves if the output property is derived or not. Args: - is_derived (bool): The flag indicating whether the output property is derived or not. - outputs_ref (_type_): The reference to the `BaseOutputs` section from which the output property was derived. + outputs_ref (_type_): The reference to the `Outputs` section from which the output property was derived. Returns: - Optional[bool]: The flag indicating whether the output property is derived or not, or whether there are missing references exists (returns None). + bool: The flag indicating whether the output property is derived or not. """ - if not is_derived: - if outputs_ref is not None: - return True - return False - elif is_derived and outputs_ref is not None: + if outputs_ref is not None: return True - return None + return False def normalize(self, archive, logger) -> None: super().normalize(archive, logger) - # Check if the output property `is_derived` or not, or if there are missing references. - check_derived = self.check_is_derived(self.is_derived, self.outputs_ref) - if check_derived is not None: - self.is_derived = check_derived - else: - logger.error( - 'A derived output property must have a reference to another `Outputs` section.' - ) - return + # Resolve if the output property `is_derived` or not. + self.is_derived = self.resolve_is_derived(self.outputs_ref) class SCFOutputs(Outputs): @@ -143,15 +131,7 @@ class SCFOutputs(Outputs): `Simulation` entry in NOMAD contains the final output properties and all the SCF steps. """ - n_scf_steps = Quantity( - type=np.int32, - description=""" - Number of self-consistent steps to converge the output property. Note that the SCF steps belong to - the same minimal `Simulation` workflow entry which is known as `SinglePoint`. - """, - ) - - scf_step = SubSection( + scf_steps = SubSection( sub_section=Outputs.m_def, repeats=True, description=""" @@ -176,30 +156,7 @@ class SCFOutputs(Outputs): """, ) - # TODO add more functionality to automatically check convergence from `self_consistency_ref` and the last `scf_step[-1]` - def check_is_scf_converged( - self, is_scf_converged: bool, logger: BoundLogger - ) -> bool: - """ - Check if the output property is converged or not. - - Args: - is_converged (bool): The flag indicating whether the output property is converged or not. - logger (BoundLogger): The logger to log messages. - - Returns: - (bool): The flag indicating whether the output property is converged or not. - """ - if not is_scf_converged: - # ? It will be nice if some of this logger messages can be checked or be used when querying data - logger.info('The output property is not converged after the SCF process.') - return False - return True + # TODO add more functionality to automatically check convergence from `self_consistency_ref` and the last two `scf_steps` def normalize(self, archive, logger) -> None: super().normalize(archive, logger) - - # Set if the output property `is_converged` or not. - self.is_scf_converged = self.check_is_scf_converged( - self.is_scf_converged, logger - ) diff --git a/tests/test_outputs.py b/tests/test_outputs.py index 97ef94be..6bc26f7e 100644 --- a/tests/test_outputs.py +++ b/tests/test_outputs.py @@ -20,55 +20,27 @@ from . import logger -from nomad_simulations.outputs import Outputs, SCFOutputs +from nomad_simulations.outputs import Outputs -class TestBaseOutputs: +class TestOutputs: """ - Test the `BaseOutputs` class defined in `outputs.py`. + Test the `Outputs` class defined in `outputs.py`. """ @pytest.mark.parametrize( - 'is_derived, outputs_ref, result', + 'outputs_ref, result', [ - (False, Outputs(), True), - (False, None, False), - (True, Outputs(), True), - (True, None, None), + (Outputs(), True), + (None, False), ], ) - def test_normalize(self, is_derived, outputs_ref, result): + def test_normalize(self, outputs_ref, result): """ - Test the `normalize` and `check_is_derived` methods. + Test the `normalize` and `resolve_is_derived` methods. """ outputs = Outputs() - assert outputs.check_is_derived(is_derived, outputs_ref) == result - outputs.is_derived = is_derived + assert outputs.resolve_is_derived(outputs_ref) == result outputs.outputs_ref = outputs_ref outputs.normalize(None, logger) - if result is not None: - assert outputs.is_derived == result - - -class TestOutputs: - """ - Test the `Outputs` class defined in `outputs.py`. - """ - - @pytest.mark.parametrize( - 'is_scf_converged, result', - [ - (False, False), - (True, True), - ], - ) - def test_normalize(self, is_scf_converged, result): - """ - Test the `normalize` method. - """ - scf_outputs = SCFOutputs() - # ! This testing is repetivite, but `check_is_scf_converged` should eventually contain more complex logic and be separated in its own testing method. - assert scf_outputs.check_is_scf_converged(is_scf_converged, logger) == result - scf_outputs.is_scf_converged = is_scf_converged - scf_outputs.normalize(None, logger) - assert scf_outputs.is_scf_converged == result + assert outputs.is_derived == result