Remove nested boost::optionals in ISISReflectometry - ornl-next (#39260)

* Display region selectors on Reflectometry GUI Preview Tab slice viewer plot on workspace load (#38838)

* Add region selectors to slice viewer plot when have matching settings

When loading a workspace or updating the angle on the preview tab,
display region selectors on the slice viewer plot for any matching
experiment settings from the look-up table.

* Remove method that is no longer used

* Add release notes for issue 36573

* Update preview tab manual testing instructions

* Remove nested boost::optionals in ISISReflectometry (#39223)

* Add validatorT typedef as a std::pair of boost::optional<T> and bool to avoid the need for nested boost:optional

* Refactor variadic template functions to validate all table fields are properly initialized, and use simpler fold syntax to check validity of all rows

* Change boost optional in common classes

* - Changed boost to std optional in batch classes
- Change boost::optional<T&> to std::optional<std::reference_wrapper<T>>

* - Changed boost to std optional in decoder/encoder

* - Changed boost to std optional in experiment and preview classes

* - Changed boost to std optional in Runs and RunsTable

* - Changed boost to std optional in Reduction

* - Make tests compatible with changes to std::optional in IsisReflectometry

* - Change more boost optional in ISIS Reflectometry

* - Remove unused allinitialized file

* Fix a bunch of cppcheck suppresions

* Move TaggetOptional out of validation result

* remove unnecesary include

* Make sure boost::optional contains values before initializing clipboard

* Move TaggedOptional into ParseReflectometryStrings

* Fix broken scale factor test

* Remove unnecesary creation of optionals for the row objects  and rename variables to more appropriate names as nested boost optional is no longer used

* Add extra check for std::nullopt in parse scale factor test

---------

Co-authored-by: rbauststfc <38210467+rbauststfc@users.noreply.github.com>
Co-authored-by: Adri Diaz <146007827+adriazalvarez@users.noreply.github.com>

Author: 3 people

buildconfig/CMake/CppCheck_Suppressions.txt.in

@@ -488,7 +488,6 @@ constVariablePointer:${CMAKE_SOURCE_DIR}/Framework/WorkflowAlgorithms/src/HFIRLo
 unreadVariable:${CMAKE_SOURCE_DIR}/Framework/WorkflowAlgorithms/src/SofTwoThetaTOF.cpp:196
 missingOverride:${CMAKE_SOURCE_DIR}/Testing/Tools/cxxtest/cxxtest/GlobalFixture.h:22
 constParameterReference:${CMAKE_SOURCE_DIR}/qt/icons/src/Icon.cpp:80
-constVariableReference:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/Common/Clipboard.cpp:80
 passedByValue:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/Common/OptionDefaults.h:47
 missingOverride:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/Common/IReflMessageHandler.h:21
 constParameterReference:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/Experiment/QtExperimentView.cpp:267
@@ -505,16 +504,9 @@ constParameterReference:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflect
 constParameterReference:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/Instrument/QtInstrumentView.cpp:91
 constParameterReference:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/Instrument/QtInstrumentView.cpp:95
 missingOverride:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/Options/OptionsDialogModel.h:20
-missingOverride:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/Preview/PreviewModel.h:31
 missingOverride:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/Runs/IRunsView.h:66
 missingOverride:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/Runs/QtCatalogSearcher.h:32
 missingOverride:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/RunsTable/IRunsTableView.h:31
-constVariablePointer:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/RunsTable/RunsTablePresenter.cpp:510
-constVariableReference:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/RunsTable/RunsTablePresenter.cpp:749
-constVariableReference:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/RunsTable/RunsTablePresenter.cpp:754
-constVariableReference:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/RunsTable/RunsTablePresenter.cpp:785
-unreadVariable:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/RunsTable/RunsTablePresenter.cpp:786
-constVariableReference:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/RunsTable/RunsTablePresenter.cpp:788
 virtualCallInConstructor:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/RunsTable/RunsTablePresenter.h:60
 missingOverride:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/Save/SaveAlgorithmRunner.h:18
 returnByReference:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/Reduction/Experiment.h:54

docs/source/release/v6.13.0/Reflectometry/New_features/36573.rst

@@ -0,0 +1 @@
+-  When a workspace is loaded on the ISIS Reflectometry GUI :ref:`Reduction Preview <refl_preview>` tab, region selectors are now added to the slice viewer plot to display any ROIs from matching experiment settings.

qt/python/mantidqt/mantidqt/widgets/regionselector/presenter.py

@@ -4,7 +4,7 @@
 #   Institut Laue - Langevin & CSNS, Institute of High Energy Physics, CAS
 # SPDX - License - Identifier: GPL - 3.0 +
 #  This file is part of the mantid workbench.
-from typing import Callable
+from typing import Callable, Optional
 
 from .view import RegionSelectorView
 from ..observers.observing_presenter import ObservingPresenter
@@ -53,7 +53,7 @@ def __init__(self, ws=None, parent=None, view=None, image_info_widget=None):
 
         self.notifyee = None
         self.view = view if view else RegionSelectorView(self, parent, image_info_widget=image_info_widget)
-        super().__init__(ws, self.view._data_view)
+        super().__init__(ws, self.view.data_view)
         self._selectors: list[Selector] = []
         self._drawing_region = False
 
@@ -143,7 +143,7 @@ def add_rectangular_region(self, region_type: str, color: str, hatch: str):
         if self._drawing_region:
             self._selectors.pop()
 
-        self._selectors.append(Selector(region_type, color, hatch, self.view._data_view.ax, self._on_rectangle_selected))
+        self._selectors.append(self._create_selector(region_type, color, hatch))
 
         self._drawing_region = True
 
@@ -155,6 +155,34 @@ def get_region(self, region_type):
                 result.extend([selector.extents[2], selector.extents[3]])
         return result
 
+    def display_rectangular_region(self, region_type: str, color: str, hatch: str, y1: float, y2: float):
+        """
+        Add a rectangular region selector to the given location on the plot, if it does not already exist
+        and if it is located within the y-axis bounds.
+        This method currently only takes y values and will automatically set some x values.
+        """
+
+        def is_same_location(extents):
+            return self._equal_within_tolerance(extents[2], y1) and self._equal_within_tolerance(extents[3], y2)
+
+        existing_selector = self._find_selector_if(lambda x: x.region_type() == region_type and is_same_location(x.extents))
+        if existing_selector:
+            return
+
+        y_min, y_max = self.view.data_view.ax.get_ybound()
+        if y1 < y_min or y2 > y_max:
+            return
+
+        # Set some values for x1 and x2
+        x_min, x_max = self.view.data_view.ax.get_xbound()
+        x_width = (x_max - x_min) / 4
+        x1 = x_min + x_width
+        x2 = x1 + x_width
+
+        selector = self._create_selector(region_type, color, hatch, (x1, x2, y1, y2))
+        selector.set_active(False)
+        self._selectors.append(selector)
+
     def _initialise_dimensions(self, workspace):
         self.view.create_dimensions(dims_info=Dimensions.get_dimensions_info(workspace))
         self.view.create_axes_orthogonal(redraw_on_zoom=not WorkspaceInfo.can_support_dynamic_rebinning(workspace))
@@ -191,7 +219,7 @@ def _remove_selector(self, selector: Selector) -> None:
         if self.notifyee:
             self.notifyee.notifyRegionChanged()
 
-    def _find_selector_if(self, predicate: Callable) -> Selector:
+    def _find_selector_if(self, predicate: Callable) -> Optional[Selector]:
         """
         Find the first selector which agrees with a predicate. Return None if no selector is found
         :param predicate: A callable function or lambda that takes a Selector and returns a boolean
@@ -201,6 +229,10 @@ def _find_selector_if(self, predicate: Callable) -> Selector:
                 return selector
         return None
 
+    @staticmethod
+    def _equal_within_tolerance(val1, val2, tolerance=1e-8):
+        return abs(val1 - val2) < tolerance
+
     @staticmethod
     def _contains_point(extents, x, y) -> bool:
         """
@@ -213,6 +245,14 @@ def _contains_point(extents, x, y) -> bool:
             return False
         return extents[0] <= x <= extents[1] and extents[2] <= y <= extents[3]
 
+    def _create_selector(
+        self, region_type: str, color: str, hatch: str, extents: Optional[tuple[float, float, float, float]] = None
+    ) -> Selector:
+        selector = Selector(region_type, color, hatch, self.view.data_view.ax, self._on_rectangle_selected)
+        if extents:
+            selector.extents = extents
+        return selector
+
     def get_extra_image_info_columns(self, xdata, ydata):
         return {}
 

qt/python/mantidqt/mantidqt/widgets/regionselector/test/test_regionselector_presenter.py

@@ -18,7 +18,7 @@ def setUp(self) -> None:
         self.patched_deps = self._ws_info_patcher.start()
         self.patched_deps["WorkspaceInfo"].get_ws_type.return_value = WS_TYPE.MATRIX
         self.mock_view = MagicMock()
-        self.mock_view._data_view.ax._get_aspect_ratio.return_value = 1
+        self.mock_view.data_view.ax._get_aspect_ratio.return_value = 1
 
     def tearDown(self) -> None:
         self._ws_info_patcher.stop()
@@ -308,7 +308,67 @@ def test_cancel_drawing_region_with_no_selectors_does_not_crash(self):
         region_selector = RegionSelector(ws=Mock(), view=Mock())
         region_selector.cancel_drawing_region()
 
-    def _mock_selectors(self, selector_one_type="signal", selector_two_type="signal"):
+    def test_display_rectangular_region_creates_selector(self):
+        region_selector = RegionSelector(ws=Mock(), view=self._mock_view_with_axes_limits((2000, 10000), (0, 500)))
+        # The expected extents should be within the axes x and y limits
+        expected_extents = (4000, 6000, 100, 300)
+        region_type = "test"
+
+        region_selector.display_rectangular_region(region_type, "black", "/", expected_extents[2], expected_extents[3])
+
+        self.assertEqual(1, len(region_selector._selectors))
+        self._check_rectangular_region(region_selector._selectors[0], region_type, expected_extents)
+
+    def test_display_rectangular_region_y1_out_of_bounds_does_not_add_selector(self):
+        y_limits = (200, 500)
+        region_selector = RegionSelector(ws=Mock(), view=self._mock_view_with_axes_limits((2000, 10000), y_limits))
+
+        region_selector.display_rectangular_region("test", "black", "/", y_limits[0] - 50, y_limits[1])
+
+        self.assertEqual(0, len(region_selector._selectors))
+
+    def test_display_rectangular_region_y2_out_of_bounds_does_not_add_selector(self):
+        y_limits = (200, 500)
+        region_selector = RegionSelector(ws=Mock(), view=self._mock_view_with_axes_limits((2000, 10000), y_limits))
+
+        region_selector.display_rectangular_region("test", "black", "/", y_limits[0], y_limits[1] + 50)
+
+        self.assertEqual(0, len(region_selector._selectors))
+
+    def test_display_rectangular_region_does_not_add_duplicate_selector(self):
+        region_selector = RegionSelector(ws=Mock(), view=self._mock_view_with_axes_limits((2000, 10000), (0, 500)))
+        # The expected extents should be within the axes x and y limits
+        expected_extents = (4000, 6000, 100, 300)
+        region_type = "test"
+        color = "black"
+        hatch = "/"
+
+        region_selector.display_rectangular_region(region_type, color, hatch, expected_extents[2], expected_extents[3])
+        self.assertEqual(1, len(region_selector._selectors))
+        region_selector.display_rectangular_region(region_type, color, hatch, expected_extents[2], expected_extents[3])
+
+        self.assertEqual(1, len(region_selector._selectors))
+        self._check_rectangular_region(region_selector._selectors[0], region_type, expected_extents)
+
+    def test_display_rectangular_region_adds_second_different_selector(self):
+        region_selector = RegionSelector(ws=Mock(), view=self._mock_view_with_axes_limits((2000, 10000), (0, 500)))
+        # The expected extents should be within the axes x and y limits
+        expected_x_values = (4000, 6000)
+        first_y_values = (100, 200)
+        second_y_values = (300, 400)
+        region_type = "test"
+        color = "black"
+        hatch = "/"
+
+        region_selector.display_rectangular_region(region_type, color, hatch, first_y_values[0], first_y_values[1])
+        region_selector.display_rectangular_region(region_type, color, hatch, second_y_values[0], second_y_values[1])
+
+        self.assertEqual(2, len(region_selector._selectors))
+        for i, expected_y_values in enumerate([first_y_values, second_y_values]):
+            self._check_rectangular_region(region_selector._selectors[i], region_type, expected_x_values + expected_y_values)
+
+    @staticmethod
+    def _mock_selectors(selector_one_type="signal", selector_two_type="signal"):
         selector_one, selector_two = Mock(), Mock()
         selector_one.region_type = Mock(return_value=selector_one_type)
         selector_two.region_type = Mock(return_value=selector_two_type)
@@ -322,6 +382,21 @@ def _mock_selectors(self, selector_one_type="signal", selector_two_type="signal"
 
         return region_selector, selector_one, selector_two
 
+    @staticmethod
+    def _mock_view_with_axes_limits(x_limits: tuple[float, float], y_limits: tuple[float, float]):
+        mock_view = Mock()
+        mock_view.data_view.ax.get_xbound.return_value = mock_view.data_view.ax.get_xlim.return_value = x_limits
+        mock_view.data_view.ax.get_ybound.return_value = mock_view.data_view.ax.get_ylim.return_value = y_limits
+        mock_view.data_view.ax._get_aspect_ratio.return_value = 1
+        return mock_view
+
+    def _check_rectangular_region(
+        self, selector: RegionSelector, expected_region_type: str, expected_extents: tuple[float, float, float, float], is_active=False
+    ):
+        self.assertEqual(is_active, selector.active)
+        self.assertEqual(expected_region_type, selector.region_type())
+        self.assertTupleEqual(expected_extents, selector.extents)
+
 
 if __name__ == "__main__":
     unittest.main()

qt/python/mantidqt/mantidqt/widgets/regionselector/view.py

@@ -28,6 +28,10 @@ def __init__(self, presenter, parent=None, dims_info=None, image_info_widget=Non
 
         self.setWindowTitle("Region Selector")
 
+    @property
+    def data_view(self):
+        return self._data_view
+
     def set_workspace(self, workspace):
         self._data_view.image_info_widget.setWorkspace(workspace)
 

qt/scientific_interfaces/ISISReflectometry/Common/Clipboard.cpp

@@ -13,13 +13,13 @@
 
 namespace MantidQt::CustomInterfaces::ISISReflectometry {
 
-Clipboard::Clipboard() : m_subtrees(boost::none), m_subtreeRoots(boost::none) {}
+Clipboard::Clipboard() : m_subtrees(std::nullopt), m_subtreeRoots(std::nullopt) {}
 
-Clipboard::Clipboard(boost::optional<std::vector<MantidQt::MantidWidgets::Batch::Subtree>> subtrees,
-                     boost::optional<std::vector<MantidQt::MantidWidgets::Batch::RowLocation>> subtreeRoots)
-    : m_subtrees(std::move(subtrees)), m_subtreeRoots(std::move(subtreeRoots)) {}
+Clipboard::Clipboard(std::vector<MantidQt::MantidWidgets::Batch::Subtree> subtrees,
+                     std::vector<MantidQt::MantidWidgets::Batch::RowLocation> subtreeRoots)
+    : m_subtrees(std::optional(subtrees)), m_subtreeRoots(std::optional(subtreeRoots)) {}
 
-bool Clipboard::isInitialized() const { return m_subtrees.is_initialized() && m_subtreeRoots.is_initialized(); }
+bool Clipboard::isInitialized() const { return m_subtrees.has_value() && m_subtreeRoots.has_value(); }
 
 int Clipboard::numberOfRoots() const {
   if (!isInitialized())
@@ -77,30 +77,30 @@ Group Clipboard::createGroupForRoot(int rootIndex) const {
 
   auto result = Group(groupName(rootIndex));
   auto rowsToAdd = createRowsForRootChildren(rootIndex);
-  for (auto &row : rowsToAdd)
+  for (auto const &row : rowsToAdd)
     result.appendRow(row);
   return result;
 }
 
-std::vector<boost::optional<Row>> Clipboard::createRowsForAllRoots() const {
+std::vector<std::optional<Row>> Clipboard::createRowsForAllRoots() const {
   if (containsGroups(*this))
     throw std::runtime_error("Attempted to get row for group clipboard item");
 
-  auto result = std::vector<boost::optional<Row>>();
+  auto result = std::vector<std::optional<Row>>();
   std::for_each(subtrees().cbegin(), subtrees().cend(), [this, &result](const auto &subtree) {
     const auto rowsToAdd = createRowsForSubtree(subtree);
     std::copy(rowsToAdd.cbegin(), rowsToAdd.cend(), std::back_inserter(result));
   });
   return result;
 }
 
-std::vector<boost::optional<Row>> Clipboard::createRowsForRootChildren(int rootIndex) const {
+std::vector<std::optional<Row>> Clipboard::createRowsForRootChildren(int rootIndex) const {
   return createRowsForSubtree(subtrees()[rootIndex]);
 }
 
-std::vector<boost::optional<Row>>
+std::vector<std::optional<Row>>
 Clipboard::createRowsForSubtree(MantidQt::MantidWidgets::Batch::Subtree const &subtree) const {
-  auto result = std::vector<boost::optional<Row>>();
+  auto result = std::vector<std::optional<Row>>();
 
   for (auto const &row : subtree) {
     // Skip the root item if it is a group
@@ -111,25 +111,26 @@ Clipboard::createRowsForSubtree(MantidQt::MantidWidgets::Batch::Subtree const &s
     std::transform(row.cells().cbegin(), row.cells().cend(), std::back_inserter(cells),
                    [](MantidQt::MantidWidgets::Batch::Cell const &cell) { return cell.contentText(); });
     auto validationResult = validateRow(cells);
-    if (validationResult.isValid())
+    if (validationResult.isValid()) {
       result.emplace_back(validationResult.assertValid());
-    else
-      result.emplace_back(boost::none);
+    } else {
+      result.emplace_back(std::nullopt);
+    }
   }
 
   return result;
 }
 
-std::vector<MantidQt::MantidWidgets::Batch::Subtree> &Clipboard::mutableSubtrees() { return m_subtrees.get(); }
+std::vector<MantidQt::MantidWidgets::Batch::Subtree> &Clipboard::mutableSubtrees() { return m_subtrees.value(); }
 
-std::vector<MantidQt::MantidWidgets::Batch::Subtree> const &Clipboard::subtrees() const { return m_subtrees.get(); }
+std::vector<MantidQt::MantidWidgets::Batch::Subtree> const &Clipboard::subtrees() const { return m_subtrees.value(); }
 
 std::vector<MantidQt::MantidWidgets::Batch::RowLocation> const &Clipboard::subtreeRoots() const {
-  return m_subtreeRoots.get();
+  return m_subtreeRoots.value();
 }
 
 std::vector<MantidQt::MantidWidgets::Batch::RowLocation> &Clipboard::mutableSubtreeRoots() {
-  return m_subtreeRoots.get();
+  return m_subtreeRoots.value();
 }
 
 bool containsGroups(Clipboard const &clipboard) {

qt/scientific_interfaces/ISISReflectometry/Common/Clipboard.h

@@ -25,16 +25,16 @@ class MANTIDQT_ISISREFLECTOMETRY_DLL Clipboard {
   };
 
   Clipboard();
-  Clipboard(boost::optional<std::vector<MantidQt::MantidWidgets::Batch::Subtree>> subtrees,
-            boost::optional<std::vector<MantidQt::MantidWidgets::Batch::RowLocation>> subtreeRoots);
+  Clipboard(std::vector<MantidQt::MantidWidgets::Batch::Subtree> subtrees,
+            std::vector<MantidQt::MantidWidgets::Batch::RowLocation> subtreeRoots);
 
   bool isInitialized() const;
   int numberOfRoots() const;
   bool isGroupLocation(int rootIndex) const;
   std::string groupName(int rootIndex) const;
   void setGroupName(int rootIndex, std::string const &groupName);
   Group createGroupForRoot(int rootIndex) const;
-  std::vector<boost::optional<Row>> createRowsForAllRoots() const;
+  std::vector<std::optional<Row>> createRowsForAllRoots() const;
 
   std::vector<MantidQt::MantidWidgets::Batch::Subtree> const &subtrees() const;
   std::vector<MantidQt::MantidWidgets::Batch::Subtree> &mutableSubtrees();
@@ -44,16 +44,16 @@ class MANTIDQT_ISISREFLECTOMETRY_DLL Clipboard {
 private:
   // The subtrees for each of the roots. Note that the Rows here contain
   // relative paths
-  boost::optional<std::vector<MantidQt::MantidWidgets::Batch::Subtree>> m_subtrees;
+  std::optional<std::vector<MantidQt::MantidWidgets::Batch::Subtree>> m_subtrees;
   // The actual locations of the roots that were copied. This allows us to work
   // out the actual paths that were copied and determine whether items are rows
   // or groups in the reflectometry GUI sense. Note that these locations may
   // not be valid in the table if other edits have been made so this should
   // only be used for checking whether copied values were rows/groups.
-  boost::optional<std::vector<MantidQt::MantidWidgets::Batch::RowLocation>> m_subtreeRoots;
+  std::optional<std::vector<MantidQt::MantidWidgets::Batch::RowLocation>> m_subtreeRoots;
 
-  std::vector<boost::optional<Row>> createRowsForRootChildren(int rootIndex) const;
-  std::vector<boost::optional<Row>> createRowsForSubtree(MantidQt::MantidWidgets::Batch::Subtree const &subtree) const;
+  std::vector<std::optional<Row>> createRowsForRootChildren(int rootIndex) const;
+  std::vector<std::optional<Row>> createRowsForSubtree(MantidQt::MantidWidgets::Batch::Subtree const &subtree) const;
 };
 
 bool MANTIDQT_ISISREFLECTOMETRY_DLL containsGroups(Clipboard const &clipboard);