Investigate unused params (#575)

* Add reporting of usage of Updatables
* Adapt tests to new error messages
* Add tests and improve message for un-updated ParamsMap
* Remove accidentally added import
* Make pylint ignore line in test
* Bump version number for new release

Author: marcpaterno

firecrown/parameters.py

@@ -7,10 +7,21 @@
 from __future__ import annotations
 
 import copy
+from dataclasses import dataclass
 import warnings
 from collections.abc import Iterable, Iterator, Sequence
 
 
+@dataclass
+class UpdatableUsageRecord:
+    """Dataclass to record the usage of parameters by an Updatable object."""
+
+    cls: str
+    prefix: str | None
+    sampler_params: list[str]
+    internal_params: list[str]
+
+
 def parameter_get_full_name(prefix: None | str, param: str) -> str:
     """Form a full parameter name from the given (optional) prefix and name.
 
@@ -78,6 +89,7 @@ def __init__(self, *args, **kwargs) -> None:
 
         self.lower_case: bool = False
         self.used_keys: set[str] = set()
+        self.usages: list[UpdatableUsageRecord] = []
 
     def __getitem__(self, key: str) -> float:
         """Return the value for the given key.
@@ -220,15 +232,65 @@ def keys(self) -> set[str]:
         """
         return set(self.params.keys())
 
+    def record_usage(
+        self,
+        cls: type,
+        prefix: str | None,
+        sampler_params: list[str],
+        internal_params: list[str],
+    ) -> None:
+        """Record the usage of parameters based on the provided lists.
+
+        This method marks parameters as used based on the provided lists
+        of sampler and internal parameters.
+
+        :param cls: type of the updatable being recorded.
+        :param prefix: optional prefix for the parameters
+        :param sampler_params: list of sampler parameter names
+        :param internal_params: list of internal parameter names
+        """
+        self.usages.append(
+            UpdatableUsageRecord(
+                cls=cls.__name__,
+                prefix=prefix,
+                sampler_params=sampler_params,
+                internal_params=internal_params,
+            )
+        )
+
+    def report_usages(self) -> str:
+        """Generate a report of parameter usages by Updatable objects.
+
+        :return: A formatted string report of parameter usages.
+        """
+        if not self.usages:
+            return "No Updatables have been updated."
+        report_lines = ["Parameter usage report:"]
+        for usage in self.usages:
+            report_lines.append(f"Updatable class: {usage.cls}, Prefix: {usage.prefix}")
+            if usage.sampler_params:
+                report_lines.append(
+                    f"  Sampler parameters used: {', '.join(usage.sampler_params)}"
+                )
+            if usage.internal_params:
+                report_lines.append(
+                    f"  Internal parameters used: {', '.join(usage.internal_params)}"
+                )
+        return "\n".join(report_lines)
+
 
 def handle_unused_params(params: ParamsMap, raise_on_unused: bool = False):
     """Check for unused keys in the parameters map."""
     unused_keys = params.get_unused_keys()
     if unused_keys:
         message = (
-            f"Unused keys in parameters: {sorted(unused_keys)}. "
-            "This may indicate a problem with the parameter mapping."
+            f"Unused keys in parameters: {sorted(unused_keys)}.\n"
+            "This may indicate a missing statistic or systematic.\n"
+            "You may also have a modeling mismatch, e.g. you have specified "
+            "sampling of neutrino masses but did not configure CAMB to use "
+            "massive neutrinos.\n"
         )
+        message += params.report_usages()
         if raise_on_unused:
             raise ValueError(message)
 

firecrown/updatable.py

@@ -193,6 +193,12 @@ def update(self, params: ParamsMap) -> None:
                 f"update, but {','.join(internal_params)} was specified."
             )
 
+        params.record_usage(
+            type(self),
+            self.parameter_prefix,
+            [sp.name for sp in self._sampler_parameters],
+            list(self._internal_parameters.keys()),
+        )
         for parameter in self._sampler_parameters:
             try:
                 value = params.get_from_full_name(parameter.fullname)

firecrown/version.py

@@ -8,5 +8,5 @@
 
 FIRECROWN_MAJOR = 1
 FIRECROWN_MINOR = 14
-FIRECROWN_PATCH = "0a0"
+FIRECROWN_PATCH = 0
 __version__ = f"{FIRECROWN_MAJOR}.{FIRECROWN_MINOR}.{FIRECROWN_PATCH}"

tests/test_parameters.py

@@ -437,20 +437,14 @@ def test_handle_unused_params():
     # All parameters are unused, should raise a warning
     with pytest.warns(
         UserWarning,
-        match=re.escape(
-            "Unused keys in parameters: ['a', 'b', 'c']. "
-            "This may indicate a problem with the parameter mapping."
-        ),
+        match=re.escape("Unused keys in parameters: ['a', 'b', 'c']."),
     ):
         handle_unused_params(params=params, raise_on_unused=False)
 
     # All parameters are unused, should raise an error
     with pytest.raises(
         ValueError,
-        match=re.escape(
-            "Unused keys in parameters: ['a', 'b', 'c']. "
-            "This may indicate a problem with the parameter mapping."
-        ),
+        match=re.escape("Unused keys in parameters: ['a', 'b', 'c']."),
     ):
         handle_unused_params(params=params, raise_on_unused=True)
 
@@ -461,24 +455,126 @@ def test_handle_unused_params():
     # Now 'c' is unused, should raise a warning
     with pytest.warns(
         UserWarning,
-        match=re.escape(
-            "Unused keys in parameters: ['c']. "
-            "This may indicate a problem with the parameter mapping."
-        ),
+        match=re.escape("Unused keys in parameters: ['c']."),
     ):
         handle_unused_params(params=params, raise_on_unused=False)
 
     # Now 'c' is unused, should raise an error
     with pytest.raises(
         ValueError,
-        match=re.escape(
-            "Unused keys in parameters: ['c']. "
-            "This may indicate a problem with the parameter mapping."
-        ),
+        match=re.escape("Unused keys in parameters: ['c']."),
     ):
         handle_unused_params(params=params, raise_on_unused=True)
 
 
+def test_report_empty_usage():
+    """Test that report_usages generates correct report for no usage of Updatable"""
+    # Test case 1: Empty usages list
+    params = ParamsMap({"a": 1.0})
+    report = params.report_usages()
+    assert report == "No Updatables have been updated."
+
+
+def test_report_usages():
+    """Test that report_usages generates correct reports for various scenarios."""
+    # Test case 2: Usage with only sampler parameters
+    params = ParamsMap({"param1": 1.0, "param2": 2.0})
+    params.record_usage(
+        cls=ParamsMap,
+        prefix="test",
+        sampler_params=["param1", "param2"],
+        internal_params=[],
+    )
+    report = params.report_usages()
+    assert "Parameter usage report:" in report
+    assert "Updatable class: ParamsMap, Prefix: test" in report
+    assert "Sampler parameters used: param1, param2" in report
+    assert "Internal parameters used:" not in report
+
+    # Test case 3: Usage with only internal parameters
+    params = ParamsMap({"internal1": 1.0, "internal2": 2.0})
+    params.record_usage(
+        cls=ParamsMap,
+        prefix="test",
+        sampler_params=[],
+        internal_params=["internal1", "internal2"],
+    )
+    report = params.report_usages()
+    assert "Parameter usage report:" in report
+    assert "Updatable class: ParamsMap, Prefix: test" in report
+    assert "Sampler parameters used:" not in report
+    assert "Internal parameters used: internal1, internal2" in report
+
+    # Test case 4: Usage with both sampler and internal parameters
+    params = ParamsMap({"s1": 1.0, "s2": 2.0, "i1": 3.0, "i2": 4.0})
+    params.record_usage(
+        cls=ParamsMap,
+        prefix="prefix1",
+        sampler_params=["s1", "s2"],
+        internal_params=["i1", "i2"],
+    )
+    report = params.report_usages()
+    assert "Parameter usage report:" in report
+    assert "Updatable class: ParamsMap, Prefix: prefix1" in report
+    assert "Sampler parameters used: s1, s2" in report
+    assert "Internal parameters used: i1, i2" in report
+
+    # Test case 5: Multiple usages with mixed scenarios
+    params = ParamsMap(
+        {
+            "a": 1.0,
+            "b": 2.0,
+            "c": 3.0,
+            "d": 4.0,
+            "e": 5.0,
+            "f": 6.0,
+        }
+    )
+    # First usage: both types
+    params.record_usage(
+        cls=ParamsMap,
+        prefix="prefix1",
+        sampler_params=["a", "b"],
+        internal_params=["c"],
+    )
+    # Second usage: only sampler
+    params.record_usage(
+        cls=ParamsMap,
+        prefix="prefix2",
+        sampler_params=["d"],
+        internal_params=[],
+    )
+    # Third usage: only internal
+    params.record_usage(
+        cls=ParamsMap,
+        prefix=None,
+        sampler_params=[],
+        internal_params=["e", "f"],
+    )
+
+    report = params.report_usages()
+    lines = report.split("\n")
+
+    # Check header
+    assert lines[0] == "Parameter usage report:"
+
+    # Check first usage
+    assert "Updatable class: ParamsMap, Prefix: prefix1" in report
+    assert "Sampler parameters used: a, b" in report
+    assert "Internal parameters used: c" in report
+
+    # Check second usage
+    assert "Updatable class: ParamsMap, Prefix: prefix2" in report
+    assert "Sampler parameters used: d" in report
+
+    # Check third usage (with None prefix)
+    assert "Updatable class: ParamsMap, Prefix: None" in report
+    assert "Internal parameters used: e, f" in report
+
+    # Verify we have the expected number of usage records
+    assert len(params.usages) == 3
+
+
 def test_params_map_union():
     p1 = ParamsMap({"a": 1.0})
     p2 = ParamsMap({"b": 2.0})

tests/test_updatable.py

@@ -20,18 +20,18 @@
 class MinimalUpdatable(Updatable):
     """A concrete time that implements Updatable."""
 
-    def __init__(self):
+    def __init__(self, prefix: str | None = None):
         """Initialize object with defaulted value."""
-        super().__init__()
+        super().__init__(prefix)
         self.a = parameters.register_new_updatable_parameter(default_value=1.0)
 
 
 class SimpleUpdatable(Updatable):
     """A concrete type that implements Updatable."""
 
-    def __init__(self):
+    def __init__(self, prefix: str | None = None):
         """Initialize object with defaulted values."""
-        super().__init__()
+        super().__init__(prefix)
 
         self.x = parameters.register_new_updatable_parameter(default_value=2.0)
         self.y = parameters.register_new_updatable_parameter(default_value=3.0)
@@ -54,6 +54,23 @@ def _get_derived_parameters(self) -> DerivedParameterCollection:
         return derived_parameters
 
 
+def test_updatable_reports():
+    su = SimpleUpdatable("bob")
+    mu = MinimalUpdatable("larry")
+    su.mu = mu  # pylint: disable=attribute-defined-outside-init
+
+    params = ParamsMap({"bob_x": 1.0, "bob_y": 2.0, "larry_a": 3.0})
+    before_use = params.report_usages()
+
+    assert before_use == "No Updatables have been updated."
+    su.update(params)
+    after_use = params.report_usages()
+    assert "Updatable class: SimpleUpdatable, Prefix: bob" in after_use
+    assert "Sampler parameters used: x, y" in after_use
+    assert "Updatable class: MinimalUpdatable, Prefix: larry" in after_use
+    assert "Sampler parameters used: a" in after_use
+
+
 def test_get_params_names():
     obj = SimpleUpdatable()
     found_names = obj.get_params_names()

tutorial/_quarto.yml

@@ -63,7 +63,7 @@ format:
 
 reference-location: margin
 citation-location: margin
-subtitle: "version 1.14.0a0"
+subtitle: "version 1.14.0"
 authors:
   - Marc Paterno
   - Sandro Vitenti