General minor fixes after meeting (#37)

* Fixes in outputs.py and testing

* Improved SelfConsistency

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`.
         """,
     )
 

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
-        )

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