From 46b90f49475605004e6e0b1c0cddf3d99df6b941 Mon Sep 17 00:00:00 2001 From: James Kerns Date: Tue, 12 Nov 2024 16:51:15 -0600 Subject: [PATCH 1/2] don't overwrite metrics of the same name --- docs/source/changelog.rst | 14 ++++++ docs/source/topics/profiles.rst | 4 ++ pylinac/core/image.py | 12 +++-- pylinac/core/profile.py | 14 +++--- pylinac/core/utilities.py | 12 +++++ tests_basic/core/test_image_metrics.py | 32 +++++++++++++ tests_basic/core/test_profile.py | 26 +++++++++++ tests_basic/core/test_utilities.py | 63 ++++++++++++++++++++++++++ 8 files changed, 165 insertions(+), 12 deletions(-) diff --git a/docs/source/changelog.rst b/docs/source/changelog.rst index 57cdcc1a2..c6fa7d796 100644 --- a/docs/source/changelog.rst +++ b/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 ^^^^^^^ diff --git a/docs/source/topics/profiles.rst b/docs/source/topics/profiles.rst index 61214e97b..e7e7de43a 100644 --- a/docs/source/topics/profiles.rst +++ b/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: diff --git a/pylinac/core/image.py b/pylinac/core/image.py index 74561d828..7554fc30a 100644 --- a/pylinac/core/image.py +++ b/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( diff --git a/pylinac/core/profile.py b/pylinac/core/profile.py index f52da46d6..13a7a3c7d 100644 --- a/pylinac/core/profile.py +++ b/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 diff --git a/pylinac/core/utilities.py b/pylinac/core/utilities.py index 8a441e95b..76016a6aa 100644 --- a/pylinac/core/utilities.py +++ b/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 diff --git a/tests_basic/core/test_image_metrics.py b/tests_basic/core/test_image_metrics.py index 6733e1d1e..2df40b76c 100644 --- a/tests_basic/core/test_image_metrics.py +++ b/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 diff --git a/tests_basic/core/test_profile.py b/tests_basic/core/test_profile.py index 98e2afcf8..36307a394 100644 --- a/tests_basic/core/test_profile.py +++ b/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, @@ -1746,7 +1748,31 @@ 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): + array = generate_profile() + profile = FWXMProfile(array, fwxm_height=50) + 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): + array = generate_profile() + profile = FWXMProfile(array, fwxm_height=50) + 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): array = generate_profile() profile = FWXMProfile(array, fwxm_height=50) diff --git a/tests_basic/core/test_utilities.py b/tests_basic/core/test_utilities.py index 130a08085..41f28fbf7 100644 --- a/tests_basic/core/test_utilities.py +++ b/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="j@j.com") @@ -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) From 2f102b647deed29611545469c73bfdfbf326983d Mon Sep 17 00:00:00 2001 From: James Kerns Date: Thu, 14 Nov 2024 10:12:22 -0600 Subject: [PATCH 2/2] new profile util update --- tests_basic/core/test_profile.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests_basic/core/test_profile.py b/tests_basic/core/test_profile.py index 882642b8a..6488672ee 100644 --- a/tests_basic/core/test_profile.py +++ b/tests_basic/core/test_profile.py @@ -1764,16 +1764,14 @@ def calculate(self) -> Any: class TestProfilePlugins(TestCase): def test_two_metrics_same_name_dont_conflict(self): - array = generate_profile() - profile = FWXMProfile(array, fwxm_height=50) + 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): - array = generate_profile() - profile = FWXMProfile(array, fwxm_height=50) + profile = generate_profile() profile.compute(metrics=[FakeMetric()]) profile.compute(metrics=[FakeMetric()]) self.assertEqual(len(profile.metrics), 2)