From f2db9d505fea892af6358e2e13de6bee43c6f0c7 Mon Sep 17 00:00:00 2001 From: ashmeigh Date: Fri, 13 Dec 2024 14:26:28 +0000 Subject: [PATCH 1/2] removed rename and remove roi from model --- .../gui/windows/spectrum_viewer/model.py | 30 ------------- .../gui/windows/spectrum_viewer/presenter.py | 11 +++-- .../spectrum_viewer/test/model_test.py | 38 ---------------- .../spectrum_viewer/test/presenter_test.py | 44 ++++++++++++++----- 4 files changed, 40 insertions(+), 83 deletions(-) diff --git a/mantidimaging/gui/windows/spectrum_viewer/model.py b/mantidimaging/gui/windows/spectrum_viewer/model.py index 1eab85c63e2..beb79e78cf1 100644 --- a/mantidimaging/gui/windows/spectrum_viewer/model.py +++ b/mantidimaging/gui/windows/spectrum_viewer/model.py @@ -500,36 +500,6 @@ def export_spectrum_to_rits(self, path: Path, tof: np.ndarray, transmission: np. rits_data = saver.create_rits_format(tof, transmission, absorption) saver.export_to_dat_rits_format(rits_data, path) - def remove_roi(self, roi_name: str) -> None: - """ - Remove the selected ROI from the model - - @param roi_name: The name of the ROI to remove - """ - if roi_name in self._roi_ranges.keys(): - if roi_name in self.special_roi_list: - raise RuntimeError(f"Cannot remove ROI: {roi_name}") - del self._roi_ranges[roi_name] - else: - raise KeyError( - f"Cannot remove ROI {roi_name} as it does not exist. \n Available ROIs: {self._roi_ranges.keys()}") - - def rename_roi(self, old_name: str, new_name: str) -> None: - """ - Rename the selected ROI from the model - - @param old_name: The current name of the ROI - @param new_name: The new name of the ROI - @raises KeyError: If the ROI does not exist - @raises RuntimeError: If the ROI is 'all' - """ - if old_name in self._roi_ranges.keys() and new_name not in self._roi_ranges.keys(): - if old_name in self.special_roi_list: - raise RuntimeError(f"Cannot remove ROI: {old_name}") - self._roi_ranges[new_name] = self._roi_ranges.pop(old_name) - else: - raise KeyError(f"Cannot rename {old_name} to {new_name} Available:{self._roi_ranges.keys()}") - def remove_all_roi(self) -> None: """ Remove all ROIs from the model diff --git a/mantidimaging/gui/windows/spectrum_viewer/presenter.py b/mantidimaging/gui/windows/spectrum_viewer/presenter.py index 380f06387f5..92ae33c5943 100644 --- a/mantidimaging/gui/windows/spectrum_viewer/presenter.py +++ b/mantidimaging/gui/windows/spectrum_viewer/presenter.py @@ -375,8 +375,14 @@ def rename_roi(self, old_name: str, new_name: str) -> None: @param old_name: Name of the ROI to rename @param new_name: New name of the ROI """ - self.view.spectrum_widget.rename_roi(old_name, new_name) - self.model.rename_roi(old_name, new_name) + try: + self.view.spectrum_widget.rename_roi(old_name, new_name) + except RuntimeError as err: + raise RuntimeError(f"Cannot remove ROI: {old_name}") from err + except KeyError as err: + raise KeyError( + f"Cannot rename {old_name} to {new_name}. Available: {list(self.view.spectrum_widget.rois.keys())}" + ) from err def do_remove_roi(self, roi_name: str | None = None) -> None: """ @@ -391,7 +397,6 @@ def do_remove_roi(self, roi_name: str | None = None) -> None: self.model.remove_all_roi() else: self.view.spectrum_widget.remove_roi(roi_name) - self.model.remove_roi(roi_name) def handle_export_tab_change(self, index: int) -> None: self.export_mode = ExportMode(index) diff --git a/mantidimaging/gui/windows/spectrum_viewer/test/model_test.py b/mantidimaging/gui/windows/spectrum_viewer/test/model_test.py index 63e30e23f63..359c75edbf3 100644 --- a/mantidimaging/gui/windows/spectrum_viewer/test/model_test.py +++ b/mantidimaging/gui/windows/spectrum_viewer/test/model_test.py @@ -392,26 +392,6 @@ def test_WHEN_stack_value_set_THEN_can_export_returns_(self, _, image_stack, exp self.model.set_stack(image_stack) self.assertEqual(self.model.has_stack(), expected) - def test_WHEN_roi_removed_THEN_roi_name_removed_from_list_of_roi_names(self): - self.model.set_stack(generate_images()) - rois = ["roi", "new_roi"] - for roi in rois: - self.model.set_new_roi(roi) - self.assertListEqual(list(self.model._roi_ranges.keys()), ["all"] + rois) - self.model.remove_roi("new_roi") - self.assertListEqual(list(self.model._roi_ranges.keys()), ["all", "roi"]) - - def test_WHEN_remove_roi_called_with_default_roi_THEN_raise_runtime_error(self): - self.model.set_stack(generate_images()) - with self.assertRaises(RuntimeError): - self.model.remove_roi("all") - self.assertListEqual(list(self.model._roi_ranges.keys()), ["all"]) - - def test_WHEN_invalid_roi_removed_THEN_keyerror_raised(self): - self.model.set_stack(generate_images()) - with self.assertRaises(KeyError): - self.model.remove_roi("non_existent_roi") - def test_WHEN_remove_all_rois_called_THEN_all_but_default_rois_removed(self): self.model.set_stack(generate_images()) rois = ["new_roi", "new_roi_2"] @@ -421,24 +401,6 @@ def test_WHEN_remove_all_rois_called_THEN_all_but_default_rois_removed(self): self.model.remove_all_roi() self.assertListEqual(list(self.model._roi_ranges.keys()), []) - def test_WHEN_roi_renamed_THEN_roi_name_changed_in_list_of_roi_names(self): - self.model.set_stack(generate_images()) - self.model.set_new_roi("new_roi") - self.assertListEqual(list(self.model._roi_ranges.keys()), ["all", "new_roi"]) - self.model.rename_roi("new_roi", "imaging_is_the_coolest") - self.assertListEqual(list(self.model._roi_ranges.keys()), ["all", "imaging_is_the_coolest"]) - - def test_WHEN_invalid_roi_renamed_THEN_keyerror_raised(self): - self.model.set_stack(generate_images()) - with self.assertRaises(KeyError): - self.model.rename_roi("non_existent_roi", "imaging_is_the_coolest") - - def test_WHEN_default_roi_renamed_THEN_runtime_error_raised(self): - self.model.set_stack(generate_images()) - self.assertListEqual(list(self.model._roi_ranges.keys()), ["all"]) - with self.assertRaises(RuntimeError): - self.model.rename_roi("all", "imaging_is_the_coolest") - def test_WHEN_no_stack_tof_THEN_time_of_flight_none(self): # No Stack self.model.set_stack(None) diff --git a/mantidimaging/gui/windows/spectrum_viewer/test/presenter_test.py b/mantidimaging/gui/windows/spectrum_viewer/test/presenter_test.py index 6c682e70a12..bd7232ee8ce 100644 --- a/mantidimaging/gui/windows/spectrum_viewer/test/presenter_test.py +++ b/mantidimaging/gui/windows/spectrum_viewer/test/presenter_test.py @@ -259,13 +259,17 @@ def test_WHEN_do_add_roi_to_table_called_THEN_roi_added_to_table(self): self.view.add_roi_table_row.assert_called_once_with("roi_1", (255, 0, 0)) def test_WHEN_do_remove_roi_called_THEN_roi_removed(self): - self.presenter.model.set_new_roi("all") - self.presenter.view.spectrum_widget.add_roi(self.presenter.model._roi_ranges["all"], "all") - for _ in range(2): - self.presenter.do_add_roi() - self.assertEqual(["all", "roi", "roi_1"], list(self.presenter.model._roi_ranges.keys())) + self.presenter.view.spectrum_widget.roi_dict = {"all": mock.Mock(), "roi": mock.Mock(), "roi_1": mock.Mock()} + self.presenter.view.spectrum_widget.remove_roi = mock.Mock() self.presenter.do_remove_roi("roi_1") - self.assertEqual(["all", "roi"], list(self.presenter.model._roi_ranges.keys())) + + self.presenter.view.spectrum_widget.remove_roi.assert_called_once_with("roi_1") + expected_roi_dict = {"all": mock.Mock(), "roi": mock.Mock()} + actual_roi_dict = { + key: value + for key, value in self.presenter.view.spectrum_widget.roi_dict.items() if key != "roi_1" + } + self.assertEqual(expected_roi_dict.keys(), actual_roi_dict.keys()) def test_WHEN_roi_clicked_THEN_roi_updated(self): roi = SpectrumROI("themightyroi", SensibleROI()) @@ -285,18 +289,34 @@ def test_WHEN_rits_roi_clicked_THEN_rois_not_updated(self): def test_WHEN_ROI_renamed_THEN_roi_renamed(self): rois = ["all", "roi", "roi_1"] self.view.spectrum_widget.roi_dict = {roi: mock.Mock() for roi in rois} - self.presenter.model._roi_ranges = {roi: mock.Mock() for roi in rois} - self.view.spectrum_widget.rename_roi = mock.Mock() + self.view.spectrum_widget.rois = {roi: mock.Mock() for roi in rois} + self.view.spectrum_widget.rename_roi = mock.Mock( + side_effect=lambda old_name, new_name: self.view.spectrum_widget.rois.update( + {new_name: self.view.spectrum_widget.rois.pop(old_name)})) + self.presenter.rename_roi("roi_1", "new_name") self.view.spectrum_widget.rename_roi.assert_called_once_with("roi_1", "new_name") - self.assertIn("new_name", self.presenter.model._roi_ranges) - self.assertNotIn("roi_1", self.presenter.model._roi_ranges) + self.assertIn("new_name", self.view.spectrum_widget.rois) + self.assertNotIn("roi_1", self.view.spectrum_widget.rois) - @parameterized.expand([("all", ), ("roi", )]) - def test_WHEN_ROI_renamed_to_existing_name_THEN_runtimeerror(self, name): + def test_WHEN_invalid_ROI_renamed_THEN_error_raised(self): rois = ["all", "roi", "roi_1"] self.view.spectrum_widget.roi_dict = {roi: mock.Mock() for roi in rois} self.presenter.model._roi_ranges = {roi: mock.Mock() for roi in rois} + self.view.spectrum_widget.rename_roi = mock.Mock(side_effect=KeyError("Invalid ROI")) + self.view.spectrum_widget.rois = {roi: mock.Mock() for roi in rois} + with self.assertRaises(KeyError): + self.presenter.rename_roi("invalid_roi", "new_name") + + @parameterized.expand([("all", ), ("roi", )]) + def test_WHEN_ROI_renamed_to_existing_name_THEN_keyerror(self, name): + rois = ["all", "roi", "roi_1"] + self.view.spectrum_widget.roi_dict = {roi: mock.Mock() for roi in rois} + self.view.spectrum_widget.rois = {roi: mock.Mock() for roi in rois} + + self.view.spectrum_widget.rename_roi = mock.Mock(side_effect=lambda old_name, new_name: ( + (_ for _ in ()).throw(KeyError(f"Cannot rename to existing ROI: {new_name}")) + if new_name in self.view.spectrum_widget.rois else None)) with self.assertRaises(KeyError): self.presenter.rename_roi("roi", name) From 33853316c00c3031362f96a1862feca37ec0f22e Mon Sep 17 00:00:00 2001 From: ashmeigh Date: Fri, 13 Dec 2024 16:40:10 +0000 Subject: [PATCH 2/2] made suggested changes --- .../gui/windows/spectrum_viewer/presenter.py | 9 +---- .../spectrum_viewer/spectrum_widget.py | 23 +++++++---- .../spectrum_viewer/test/presenter_test.py | 27 +------------ .../spectrum_viewer/test/spectrum_test.py | 39 +++++++++++++++++-- 4 files changed, 54 insertions(+), 44 deletions(-) diff --git a/mantidimaging/gui/windows/spectrum_viewer/presenter.py b/mantidimaging/gui/windows/spectrum_viewer/presenter.py index 92ae33c5943..ee682637ff9 100644 --- a/mantidimaging/gui/windows/spectrum_viewer/presenter.py +++ b/mantidimaging/gui/windows/spectrum_viewer/presenter.py @@ -375,14 +375,7 @@ def rename_roi(self, old_name: str, new_name: str) -> None: @param old_name: Name of the ROI to rename @param new_name: New name of the ROI """ - try: - self.view.spectrum_widget.rename_roi(old_name, new_name) - except RuntimeError as err: - raise RuntimeError(f"Cannot remove ROI: {old_name}") from err - except KeyError as err: - raise KeyError( - f"Cannot rename {old_name} to {new_name}. Available: {list(self.view.spectrum_widget.rois.keys())}" - ) from err + self.view.spectrum_widget.rename_roi(old_name, new_name) def do_remove_roi(self, roi_name: str | None = None) -> None: """ diff --git a/mantidimaging/gui/windows/spectrum_viewer/spectrum_widget.py b/mantidimaging/gui/windows/spectrum_viewer/spectrum_widget.py index 72a30206038..d9439981a77 100644 --- a/mantidimaging/gui/windows/spectrum_viewer/spectrum_widget.py +++ b/mantidimaging/gui/windows/spectrum_viewer/spectrum_widget.py @@ -250,9 +250,13 @@ def remove_roi(self, roi_name: str) -> None: @param roi_name: The name of the ROI to remove. """ - if roi_name in self.roi_dict.keys() and roi_name != "all": - self.image.vb.removeItem(self.roi_dict[roi_name]) - del self.roi_dict[roi_name] + if roi_name == "all": + raise RuntimeError("Cannot remove the default 'all' ROI.") + if roi_name not in self.roi_dict: + raise KeyError(f"ROI '{roi_name}' does not exist.") + + self.image.vb.removeItem(self.roi_dict[roi_name]) + del self.roi_dict[roi_name] def rename_roi(self, old_name: str, new_name: str) -> None: """ @@ -263,10 +267,15 @@ def rename_roi(self, old_name: str, new_name: str) -> None: @param new_name: The new name of the ROI. @raise KeyError: If the new name is already in use or equal to 'roi' or 'all'. """ - if old_name in self.roi_dict.keys() and new_name not in self.roi_dict.keys(): - self.roi_dict[new_name] = self.roi_dict.pop(old_name) - self.spectrum_data_dict[new_name] = self.spectrum_data_dict.pop(old_name) - self.roi_dict[new_name].rename_roi(new_name) + + if old_name not in self.roi_dict.keys(): + raise RuntimeError(f"Cannot rename non-existent ROI: {old_name}") + if new_name in self.roi_dict.keys(): + raise KeyError(f"New ROI name '{new_name}' is invalid or already in use.") + + self.roi_dict[new_name] = self.roi_dict.pop(old_name) + self.spectrum_data_dict[new_name] = self.spectrum_data_dict.pop(old_name) + self.roi_dict[new_name].rename_roi(new_name) class CustomViewBox(ViewBox): diff --git a/mantidimaging/gui/windows/spectrum_viewer/test/presenter_test.py b/mantidimaging/gui/windows/spectrum_viewer/test/presenter_test.py index bd7232ee8ce..44513fdc48c 100644 --- a/mantidimaging/gui/windows/spectrum_viewer/test/presenter_test.py +++ b/mantidimaging/gui/windows/spectrum_viewer/test/presenter_test.py @@ -264,12 +264,6 @@ def test_WHEN_do_remove_roi_called_THEN_roi_removed(self): self.presenter.do_remove_roi("roi_1") self.presenter.view.spectrum_widget.remove_roi.assert_called_once_with("roi_1") - expected_roi_dict = {"all": mock.Mock(), "roi": mock.Mock()} - actual_roi_dict = { - key: value - for key, value in self.presenter.view.spectrum_widget.roi_dict.items() if key != "roi_1" - } - self.assertEqual(expected_roi_dict.keys(), actual_roi_dict.keys()) def test_WHEN_roi_clicked_THEN_roi_updated(self): roi = SpectrumROI("themightyroi", SensibleROI()) @@ -288,16 +282,11 @@ def test_WHEN_rits_roi_clicked_THEN_rois_not_updated(self): def test_WHEN_ROI_renamed_THEN_roi_renamed(self): rois = ["all", "roi", "roi_1"] - self.view.spectrum_widget.roi_dict = {roi: mock.Mock() for roi in rois} self.view.spectrum_widget.rois = {roi: mock.Mock() for roi in rois} - self.view.spectrum_widget.rename_roi = mock.Mock( - side_effect=lambda old_name, new_name: self.view.spectrum_widget.rois.update( - {new_name: self.view.spectrum_widget.rois.pop(old_name)})) - + self.view.spectrum_widget.rename_roi = mock.Mock() self.presenter.rename_roi("roi_1", "new_name") + self.view.spectrum_widget.rename_roi.assert_called_once_with("roi_1", "new_name") - self.assertIn("new_name", self.view.spectrum_widget.rois) - self.assertNotIn("roi_1", self.view.spectrum_widget.rois) def test_WHEN_invalid_ROI_renamed_THEN_error_raised(self): rois = ["all", "roi", "roi_1"] @@ -308,18 +297,6 @@ def test_WHEN_invalid_ROI_renamed_THEN_error_raised(self): with self.assertRaises(KeyError): self.presenter.rename_roi("invalid_roi", "new_name") - @parameterized.expand([("all", ), ("roi", )]) - def test_WHEN_ROI_renamed_to_existing_name_THEN_keyerror(self, name): - rois = ["all", "roi", "roi_1"] - self.view.spectrum_widget.roi_dict = {roi: mock.Mock() for roi in rois} - self.view.spectrum_widget.rois = {roi: mock.Mock() for roi in rois} - - self.view.spectrum_widget.rename_roi = mock.Mock(side_effect=lambda old_name, new_name: ( - (_ for _ in ()).throw(KeyError(f"Cannot rename to existing ROI: {new_name}")) - if new_name in self.view.spectrum_widget.rois else None)) - with self.assertRaises(KeyError): - self.presenter.rename_roi("roi", name) - def test_WHEN_do_remove_roi_called_with_no_arguments_THEN_all_rois_removed(self): rois = ["all", "roi", "roi_1", "roi_2"] self.view.spectrum_widget.roi_dict = {roi: mock.Mock() for roi in rois} diff --git a/mantidimaging/gui/windows/spectrum_viewer/test/spectrum_test.py b/mantidimaging/gui/windows/spectrum_viewer/test/spectrum_test.py index fee63dbf2a7..62845a14a1f 100644 --- a/mantidimaging/gui/windows/spectrum_viewer/test/spectrum_test.py +++ b/mantidimaging/gui/windows/spectrum_viewer/test/spectrum_test.py @@ -132,15 +132,46 @@ def test_WHEN_rename_roi_called_THEN_roi_renamed(self): self.assertNotIn("roi_1", self.spectrum_widget.roi_dict) self.assertIn("roi_2", self.spectrum_widget.roi_dict) - def test_WHEN_rename_roi_called_with_default_roi_THEN_roi_name_not_changed(self): + def test_WHEN_rename_roi_called_with_default_roi_THEN_keyerror_is_raised(self): spectrum_roi = SpectrumROI("roi_1", self.sensible_roi, rotatable=False, scaleSnap=True, translateSnap=True) self.spectrum_widget.roi_dict["roi"] = spectrum_roi self.spectrum_widget.roi_dict["roi_1"] = spectrum_roi self.spectrum_widget.spectrum_data_dict["roi_1"] = np.array([0, 0, 0, 0]) - self.assertIn("roi_1", self.spectrum_widget.roi_dict.keys()) - self.spectrum_widget.rename_roi("roi_1", "roi") - self.assertIn("roi_1", self.spectrum_widget.roi_dict) + with self.assertRaises(KeyError): + self.spectrum_widget.rename_roi("roi_1", "roi") def test_WHEN_tof_axis_label_changed_THEN_axis_label_set(self): self.spectrum_plot_widget.set_tof_axis_label("test") self.assertEqual(self.spectrum_plot_widget.spectrum.getAxis('bottom').labelText, "test") + + def test_WHEN_roi_removed_THEN_roi_name_removed_from_list_of_roi_names(self): + spectrum_roi = SpectrumROI("new_roi", self.sensible_roi, rotatable=False, scaleSnap=True, translateSnap=True) + self.spectrum_widget.roi_dict = {"all": mock.Mock(), "roi": mock.Mock(), "new_roi": spectrum_roi} + self.spectrum_widget.image.vb = mock.Mock() + + self.spectrum_widget.remove_roi("new_roi") + self.assertListEqual(list(self.spectrum_widget.roi_dict.keys()), ["all", "roi"]) + + def test_WHEN_remove_roi_called_with_default_roi_THEN_raise_runtime_error(self): + self.spectrum_widget.roi_dict = {"all": mock.Mock(), "roi": mock.Mock()} + with self.assertRaises(RuntimeError): + self.spectrum_widget.remove_roi("all") + + def test_WHEN_invalid_roi_removed_THEN_keyerror_raised(self): + self.spectrum_widget.roi_dict = {"all": mock.Mock(), "roi": mock.Mock()} + with self.assertRaises(KeyError): + self.spectrum_widget.remove_roi("non_existent_roi") + + def test_WHEN_rename_non_existent_roi_THEN_runtime_error(self): + self.spectrum_widget.roi_dict = {"roi_1": mock.Mock(), "roi_2": mock.Mock()} + self.spectrum_widget.spectrum_data_dict = {"roi_1": mock.Mock(), "roi_2": mock.Mock()} + + with self.assertRaises(RuntimeError): + self.spectrum_widget.rename_roi("roi_invalid", "roi_new") + + def test_WHEN_rename_to_existing_roi_name_THEN_key_error(self): + self.spectrum_widget.roi_dict = {"roi_1": mock.Mock(), "roi_2": mock.Mock()} + self.spectrum_widget.spectrum_data_dict = {"roi_1": mock.Mock(), "roi_2": mock.Mock()} + + with self.assertRaises(KeyError): + self.spectrum_widget.rename_roi("roi_1", "roi_2")