Merged in feature/uniquify-metrics (pull request #480)

don't overwrite metrics of the same name

Approved-by: Randy Taylor

docs/source/changelog.rst

@@ -13,6 +13,20 @@ Legend
 v 3.29.0
 --------
 
+Image Metrics
+^^^^^^^^^^^^^
+
+* :bdg-warning:`Fixed` Metrics passed to :meth:`~pylinac.core.image.BaseImage.compute()` of the same name are no longer overwritten.
+  A unique key is generated if the name already exists.
+
+Profile Metrics
+^^^^^^^^^^^^^^^
+
+* :bdg-warning:`Fixed` Metrics passed to :meth:`~pylinac.core.profile.ProfileBase.compute()` of the same name are no longer overwritten.
+  A unique key is generated if the name already exists.
+* :bdg-warning:`Fixed` Calling ``.compute()`` multiple times would overwrite the ``.metric_values`` and ``.metrics`` attributes.
+  This has been fixed.
+
 TRS-398
 ^^^^^^^
 

docs/source/topics/profiles.rst

@@ -786,6 +786,10 @@ edges will not accurately represent the physical size and position of the pixels
 API
 ---
 
+.. autoclass:: pylinac.core.profile.ProfileBase
+    :inherited-members:
+    :members:
+
 .. autoclass:: pylinac.core.profile.SingleProfile
     :inherited-members:
     :members:

pylinac/core/image.py

@@ -57,7 +57,7 @@
 from .plotly_utils import add_title
 from .profile import stretch as stretcharray
 from .scale import MachineScale, convert, wrap360
-from .utilities import decode_binary, is_close, simple_round
+from .utilities import decode_binary, is_close, simple_round, uniquify
 from .validators import double_dimension
 
 ARRAY = "Array"
@@ -966,11 +966,13 @@ def compute(self, metrics: list[MetricBase] | MetricBase) -> Any | dict[str, Any
             metric.inject_image(self)
             value = metric.context_calculate()
             self.metrics.append(metric)
-            metric_data[metric.name] = value
-        # TODO: use |= when 3.9 is min supported version
-        self.metric_values.update(metric_data)
+            key = uniquify(
+                list(metric_data.keys()) + list(self.metric_values.keys()), metric.name
+            )
+            metric_data[key] = value
+        self.metric_values |= metric_data
         if len(metrics) == 1:
-            return metric_data[metrics[0].name]
+            return metric_data[key]
         return metric_data
 
     def as_dicom(

pylinac/core/profile.py

@@ -34,7 +34,7 @@
 from .gamma import gamma_1d, gamma_geometric
 from .geometry import Circle, Point
 from .hill import Hill
-from .utilities import TemporaryAttribute, convert_to_enum
+from .utilities import TemporaryAttribute, convert_to_enum, uniquify
 
 # for Hill fits of 2D device data the # of points can be small.
 # This results in optimization warnings about the variance of the fit (the variance isn't of concern for us for that particular item)
@@ -558,19 +558,19 @@ def compute(
             If only one metric was given, the value of that metric is returned.
 
         """
-        self.metric_values = {}
-        self.metrics = []
         values = {}
         if isinstance(metrics, ProfileMetric):
             metrics = [metrics]
         for metric in metrics:
             metric.inject_profile(self)
             self.metrics.append(metric)
-            values[metric.full_name] = metric.calculate()
-        # TODO: use |= when 3.9 is min version
-        self.metric_values.update(values)
+            key = uniquify(
+                list(values.keys()) + list(self.metric_values.keys()), metric.full_name
+            )
+            values[key] = metric.calculate()
+        self.metric_values |= values
         if len(values) == 1:
-            return list(values.values())[0]
+            return values[key]
         else:
             return values
 

pylinac/core/utilities.py

@@ -361,3 +361,15 @@ def to_quaac(
             d.to_json_file(path)
         elif format == "yaml":
             d.to_yaml_file(path)
+
+
+def uniquify(seq: list[str] | tuple[str], value: str) -> str:
+    """Create a unique string if it already exists in the sequence"""
+    if value not in seq:
+        return value
+    i = 1
+    while True:
+        new_value = f"{value}-{i}"
+        if new_value not in seq:
+            return new_value
+        i += 1

tests_basic/core/test_image_metrics.py

@@ -19,6 +19,7 @@
     GlobalFieldLocator,
     GlobalSizedDiskLocator,
     GlobalSizedFieldLocator,
+    MetricBase,
     RectangleROIMetric,
     SizedDiskLocator,
 )
@@ -178,6 +179,37 @@ def test_min_separation_massive(self):
         self.assertEqual(len(bbs), 1)
 
 
+class FakeMetric(MetricBase):
+    name = "myfakemetric"
+
+    def calculate(self) -> Point:
+        return Point(1, 1)
+
+
+class FakeMetric2(FakeMetric):
+    pass
+
+
+class TestGeneral(TestCase):
+    def test_multiple_metrics_with_same_name_arent_overwritten(self):
+        ds = create_bb_image(bb_size=5)
+        img = DicomImage.from_dataset(ds)
+        metrics = img.compute(metrics=[FakeMetric(), FakeMetric2()])
+        self.assertEqual(len(metrics), 2)
+        self.assertEqual(len(img.metric_values), 2)
+        self.assertIn("myfakemetric", img.metric_values)
+        self.assertIn("myfakemetric-1", img.metric_values)
+
+    def test_multiple_independent_computes_dont_conflict(self):
+        ds = create_bb_image(bb_size=5)
+        img = DicomImage.from_dataset(ds)
+        img.compute(metrics=[FakeMetric()])
+        img.compute(metrics=[FakeMetric()])
+        self.assertEqual(len(img.metric_values), 2)
+        self.assertIn("myfakemetric", img.metric_values)
+        self.assertIn("myfakemetric-1", img.metric_values)
+
+
 class TestDeduplicatePoints(TestCase):
     def test_normal(self):
         # normal case: no duplicates

tests_basic/core/test_profile.py

@@ -1,4 +1,5 @@
 import warnings
+from typing import Any
 from unittest import TestCase
 
 import numpy as np
@@ -37,6 +38,7 @@
     FlatnessRatioMetric,
     PenumbraLeftMetric,
     PenumbraRightMetric,
+    ProfileMetric,
     SlopeMetric,
     SymmetryAreaMetric,
     SymmetryPointDifferenceMetric,
@@ -1753,7 +1755,29 @@ def test_electron_pdd(self):
         self.assertAlmostEqual(dmax, 20.92, delta=0.01)
 
 
+class FakeMetric(ProfileMetric):
+    name = "Fake Metric"
+
+    def calculate(self) -> Any:
+        return 1
+
+
 class TestProfilePlugins(TestCase):
+    def test_two_metrics_same_name_dont_conflict(self):
+        profile = generate_profile()
+        metrics = profile.compute(metrics=[FakeMetric(), FakeMetric()])
+        self.assertEqual(len(metrics), 2)
+        self.assertIn("Fake Metric", metrics)
+        self.assertIn("Fake Metric-1", metrics)
+
+    def test_two_independent_computes_dont_conflict(self):
+        profile = generate_profile()
+        profile.compute(metrics=[FakeMetric()])
+        profile.compute(metrics=[FakeMetric()])
+        self.assertEqual(len(profile.metrics), 2)
+        self.assertIn("Fake Metric", profile.metric_values)
+        self.assertIn("Fake Metric-1", profile.metric_values)
+
     def test_plot_without_metric_is_fine(self):
         profile = generate_profile(profile_type=FWXMProfile)
         profile.plot()

tests_basic/core/test_utilities.py

@@ -7,6 +7,7 @@
 
 import numpy as np
 import quaac
+from parameterized import parameterized
 from quaac import Attachment, Equipment, User
 
 from pylinac import Interpolation
@@ -17,6 +18,7 @@
     is_close_degrees,
     is_iterable,
     simple_round,
+    uniquify,
 )
 
 performer = User(name="James Kerns", email="[email protected]")
@@ -223,3 +225,64 @@ def test_results(self):
         data_json = instance.results_data(as_json=True)
         self.assertIsInstance(data_json, str)
         json.loads(data_json)
+
+
+class TestUniquifyFunction(unittest.TestCase):
+    @parameterized.expand(
+        [
+            (
+                "test_unique_name_not_in_existing",
+                ["apples", "bananas"],
+                "cherries",
+                "cherries",
+            ),
+            (
+                "test_unique_name_in_existing_no_suffixes",
+                ["apples", "bananas"],
+                "bananas",
+                "bananas-1",
+            ),
+            (
+                "test_unique_name_in_existing_with_suffixes",
+                ["apples", "bananas", "bananas-1", "bananas-2"],
+                "bananas",
+                "bananas-3",
+            ),
+            (
+                "test_unique_name_with_gaps_in_suffixes",
+                ["file", "file-1", "file-2", "file-4"],
+                "file",
+                "file-3",
+            ),
+            (
+                "test_unique_name_large_number_of_suffixes",
+                ["item"] + [f"item-{i}" for i in range(1, 1000)],
+                "item",
+                "item-1000",
+            ),
+            ("test_empty_existing_list", [], "unique", "unique"),
+            ("test_empty_string_name", ["", "-1", "-2"], "", "-3"),
+            (
+                "test_name_already_with_suffix",
+                ["report", "report-1", "report-2"],
+                "report-1",
+                "report-1-1",
+            ),
+            (
+                "test_name_with_multiple_dashes",
+                ["data-set", "data-set-1"],
+                "data-set",
+                "data-set-2",
+            ),
+            ("test_case_sensitivity", ["Bananas", "bananas-1"], "bananas", "bananas"),
+            (
+                "test_numeric_suffix_collision",
+                ["item", "item-1", "item-01"],
+                "item",
+                "item-2",
+            ),
+        ]
+    )
+    def test_uniquify(self, name, existing, input_name, expected):
+        result = uniquify(existing, input_name)
+        self.assertEqual(result, expected)