Merge pull request #23714 from mantidproject/23706_fix_iterators

Fix DetectorInfo and SpectrumInfo iterators

Framework/API/inc/MantidAPI/SpectrumInfoItem.h

@@ -87,12 +87,6 @@ private:
   SpectrumInfoItem(const SpectrumInfo &spectrumInfo, const size_t index)
       : m_spectrumInfo(&spectrumInfo), m_index(index) {}
 
-  // Provide copy and move constructors
-  SpectrumInfoItem(const SpectrumInfoItem &other) = default;
-  SpectrumInfoItem &operator=(const SpectrumInfoItem &rhs) = default;
-  SpectrumInfoItem(SpectrumInfoItem &&other) = default;
-  SpectrumInfoItem &operator=(SpectrumInfoItem &&rhs) = default;
-
   // Non-owning pointer. A reference makes the class unable to define an
   // assignment operator that we need.
   const SpectrumInfo *m_spectrumInfo;

Framework/Geometry/inc/MantidGeometry/Instrument/DetectorInfoItem.h

@@ -74,12 +74,6 @@ private:
   DetectorInfoItem(const DetectorInfo &detectorInfo, const size_t index)
       : m_detectorInfo(&detectorInfo), m_index(index) {}
 
-  // Provide copy and move constructors
-  DetectorInfoItem(const DetectorInfoItem &other) = default;
-  DetectorInfoItem &operator=(const DetectorInfoItem &rhs) = default;
-  DetectorInfoItem(DetectorInfoItem &&other) = default;
-  DetectorInfoItem &operator=(DetectorInfoItem &&rhs) = default;
-
   // Non-owning pointer. A reference makes the class unable to define an
   // assignment operator that we need.
   const DetectorInfo *m_detectorInfo;

Framework/PythonInterface/inc/MantidPythonInterface/api/DetectorInfoPythonIterator.h

@@ -58,13 +58,14 @@ public:
         m_firstOrDone(true) {}
 
   const DetectorInfoItem &next() {
-    if (!m_firstOrDone) {
+    if (!m_firstOrDone)
       ++m_begin;
-    } else {
+    else
       m_firstOrDone = false;
+    if (m_begin == m_end) {
+      m_firstOrDone = true;
       objects::stop_iteration_error();
     }
-
     return *m_begin;
   }
 

Framework/PythonInterface/inc/MantidPythonInterface/api/SpectrumInfoPythonIterator.h

@@ -62,10 +62,12 @@ public:
         m_firstOrDone(true) {}
 
   const SpectrumInfoItem &next() {
-    if (!m_firstOrDone) {
+    if (!m_firstOrDone)
       ++m_begin;
-    } else {
+    else
       m_firstOrDone = false;
+    if (m_begin == m_end) {
+      m_firstOrDone = true;
       objects::stop_iteration_error();
     }
     return *m_begin;

Framework/PythonInterface/mantid/api/src/Exports/SpectrumInfoItem.cpp

@@ -12,7 +12,7 @@ using namespace boost::python;
 void export_SpectrumInfoItem() {
 
   // Export to Python
-  class_<SpectrumInfoItem, boost::noncopyable>("SpectrumInfoItem", no_init)
+  class_<SpectrumInfoItem>("SpectrumInfoItem", no_init)
       .add_property("isMonitor", &SpectrumInfoItem::isMonitor)
       .add_property("isMasked", &SpectrumInfoItem::isMasked)
       .add_property("twoTheta", &SpectrumInfoItem::twoTheta)

Framework/PythonInterface/mantid/api/src/Exports/SpectrumInfoPythonIterator.cpp

@@ -22,5 +22,14 @@ void export_SpectrumInfoPythonIterator() {
 #endif
           ,
           &SpectrumInfoPythonIterator::next,
-          return_value_policy<reference_existing_object>());
+          return_value_policy<copy_const_reference>());
+  /*
+   Return value policy for next is to copy the const reference. Copy by value is
+   essential for python 2.0 compatibility because items (SpectrumInfoItem) will
+   outlive their iterators if declared as part of for loops. There is no good
+   way to deal with this other than to force a copy so that internals of the
+   item are not also corrupted. Future developers may wish to choose a separte
+   policy for python 3.0 where this is not a concern, and const ref returns
+   would be faster.
+  */
 }

Framework/PythonInterface/mantid/geometry/src/Exports/DetectorInfoItem.cpp

@@ -12,7 +12,7 @@ using namespace boost::python;
 void export_DetectorInfoItem() {
 
   // Export to Python
-  class_<DetectorInfoItem, boost::noncopyable>("DetectorInfoItem", no_init)
+  class_<DetectorInfoItem>("DetectorInfoItem", no_init)
       .add_property("isMonitor", &DetectorInfoItem::isMonitor)
       .add_property("isMasked", &DetectorInfoItem::isMasked)
       .add_property("twoTheta", &DetectorInfoItem::twoTheta)

Framework/PythonInterface/mantid/geometry/src/Exports/DetectorInfoPythonIterator.cpp

@@ -22,5 +22,14 @@ void export_DetectorInfoPythonIterator() {
 #endif
           ,
           &DetectorInfoPythonIterator::next,
-          return_value_policy<reference_existing_object>());
+          return_value_policy<copy_const_reference>());
+  /*
+   Return value policy for next is to copy the const reference. Copy by value is
+   essential for python 2.0 compatibility because items (DetectorInfoItem) will
+   outlive their iterators if declared as part of for loops. There is no good
+   way to deal with this other than to force a copy so that internals of the
+   item are not also corrupted. Future developers may wish to choose a separte
+   policy for python 3.0 where this is not a concern, and const ref returns
+   would be faster.
+  */
 }

Framework/PythonInterface/test/python/mantid/api/SpectrumInfoTest.py

@@ -12,7 +12,7 @@ class SpectrumInfoTest(unittest.TestCase):
     def setUp(self):
         """ Set up code. """
         if self.__class__._ws is None:
-            self.__class__._ws = WorkspaceCreationHelper.create2DWorkspaceWithFullInstrument(2, 1, False) # no monitors
+            self.__class__._ws = WorkspaceCreationHelper.create2DWorkspaceWithFullInstrument(3, 1, False) # no monitors
             self.__class__._ws.getSpectrum(0).clearDetectorIDs()
 
     """
@@ -26,13 +26,13 @@ class SpectrumInfoTest(unittest.TestCase):
         """ Check that the number of spectra we initailly created
             is the same as in memory. """
         info = self._ws.spectrumInfo()
-        self.assertEquals(len(info), 2)
+        self.assertEquals(len(info), 3)
 
     def test_size(self):
         """ Check that the number of spectra we initailly created
             is the same as in memory. """
         info = self._ws.spectrumInfo()
-        self.assertEquals(info.size(), 2)
+        self.assertEquals(info.size(), 3)
 
     def test_isMonitor(self):
         """ Check if a monitor is present. """
@@ -113,53 +113,41 @@ class SpectrumInfoTest(unittest.TestCase):
     Iteration
     ---------------
     """
-
-    def test_iteration_for_isMonitor(self):
-        info = self._ws.spectrumInfo()
-        for specInfo in info:
-            if specInfo.hasUniqueDetector:
-                self.assertEquals(type(specInfo.isMonitor), bool)
-
-    def test_iteration_for_isMasked(self):
-        info = self._ws.spectrumInfo()
-        for specInfo in info:
-            if specInfo.hasUniqueDetector:
-                self.assertEquals(type(specInfo.isMasked), bool)
-
-    def test_iteration_for_twoTheta(self):
+    def test_basic_iteration(self):
         info = self._ws.spectrumInfo()
-        for specInfo in info:
-            if specInfo.hasUniqueDetector:
-                self.assertEquals(type(specInfo.twoTheta), float)
+        expected_iterations = len(info) 
+        actual_iterations = len(list(iter(info)))
+        self.assertEquals(expected_iterations, actual_iterations)
 
-    def test_iteration_for_signedTwoTheta(self):
+    def test_iterator_for_monitors(self):
         info = self._ws.spectrumInfo()
-        for specInfo in info:
-            if specInfo.hasUniqueDetector:
-                self.assertEquals(type(specInfo.signedTwoTheta), float)
-
-    def test_iteration_for_l2(self):
-        info = self._ws.spectrumInfo()
-        for specInfo in info:
-            if specInfo.hasUniqueDetector:
-                self.assertEquals(type(specInfo.l2), float)
-
-    def test_iteration_for_hasUniqueDetector(self):
-        info = self._ws.spectrumInfo()
-        for specInfo in info:
-            self.assertEquals(type(specInfo.hasUniqueDetector), bool)
-
-    def test_iteration_for_spectrumDefinition(self):
+        # check no monitors in instrument
+        it = iter(info)
+        next(it) # skip first as detectors cleared
+        for item in it:
+            self.assertFalse(item.isMonitor)
+            
+    def test_iterator_for_masked(self):
         info = self._ws.spectrumInfo()
-        for specInfo in info:
-            if specInfo.hasUniqueDetector:
-                self.assertEquals(type(specInfo.spectrumDefinition), SpectrumDefinition)
+        # nothing should be masked 
+        it = iter(info)
+        next(it) # skip first as detectors cleared
+        for item in it:
+            self.assertFalse(item.isMasked)
 
     def test_iteration_for_position(self):
         info = self._ws.spectrumInfo()
-        for specInfo in info:
-            if specInfo.hasUniqueDetector:
-                self.assertEquals(type(specInfo.position), V3D)
+        lastY = None
+        it = iter(info)
+        next(it) # skip first as detectors cleared
+        for i,item in enumerate(it):
+            pos = item.position
+            # See test helper for position construction
+            self.assertAlmostEquals(pos.X(), 0)
+            self.assertAlmostEquals(pos.Z(), 5)
+            if(lastY):
+                self.assertTrue(pos.Y() > lastY)
+            lastY = pos.Y()
 
     """
     ----------------------------------------------------------------------------

Framework/PythonInterface/test/python/mantid/geometry/DetectorInfoTest.py

@@ -106,30 +106,35 @@ class DetectorInfoTest(unittest.TestCase):
     ---------------
     """
 
-    def test_iteration_for_isMonitor(self):
+    def test_basic_iteration(self):
         info = self._ws.detectorInfo()
-        for detInfo in info:
-            self.assertEquals(type(detInfo.isMonitor), bool)
+        expected_iterations = len(info) 
+        actual_iterations = len(list(iter(info)))
+        self.assertEquals(expected_iterations, actual_iterations)
 
-    def test_iteration_for_isMasked(self):
+    def test_iterator_for_monitors(self):
         info = self._ws.detectorInfo()
-        for detInfo in info:
-            self.assertEquals(type(detInfo.isMasked), bool)
-
-    def test_iteration_for_twoTheta(self):
+        # check no monitors in instrument
+        for item in info:
+            self.assertFalse(item.isMonitor)
+            
+    def test_iterator_for_masked(self):
         info = self._ws.detectorInfo()
-        for detInfo in info:
-            self.assertEquals(type(detInfo.twoTheta), float)
+        # nothing should be masked 
+        for item in info:
+            self.assertFalse(item.isMasked)
 
     def test_iteration_for_position(self):
         info = self._ws.detectorInfo()
-        for detInfo in info:
-            self.assertEquals(type(detInfo.position), V3D)
-
-    def test_iteration_for_rotation(self):
-        info = self._ws.detectorInfo()
-        for detInfo in info:
-            self.assertEquals(type(detInfo.rotation), Quat)
+        lastY = None
+        for i,item in enumerate(info):
+            pos = item.position
+            # See test helper for position construction
+            self.assertAlmostEquals(pos.X(), 0)
+            self.assertAlmostEquals(pos.Z(), 5)
+            if(lastY):
+                self.assertTrue(pos.Y() > lastY)
+            lastY = pos.Y()
 
 
     """