Merge pull request #38900 from rosswhitfield/fix_addProperty_export

Fix addProperty python export when value is a Property

Author: rosswhitfield

Framework/Kernel/inc/MantidKernel/Property.h

@@ -104,6 +104,8 @@ class MANTID_KERNEL_DLL Property {
   const std::type_info *type_info() const;
   const std::string type() const;
 
+  void setName(const std::string &name);
+
   /// Overridden function that checks whether the property, if not overriden
   /// returns ""
   virtual std::string isValid() const;

Framework/Kernel/src/Property.cpp

@@ -59,6 +59,16 @@ Property::~Property() = default;
  */
 const std::string &Property::name() const { return m_name; }
 
+/** Set the property's name
+ *  @param name :: The name of the property
+ */
+void Property::setName(const std::string &name) {
+  if (name.empty()) {
+    throw std::invalid_argument("An empty property name is not permitted");
+  }
+  m_name = name;
+}
+
 /** Get the property's documentation string
  *  @return The documentation string
  */

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

@@ -74,8 +74,15 @@ void addPropertyWithUnit(Run &self, const std::string &name, const bpl::object &
   extract<Property *> extractor(value);
   if (extractor.check()) {
     Property *prop = extractor();
-    self.addProperty(prop->clone(),
-                     replace); // Clone the property as Python owns the one that is passed in
+    // Clone the property as Python owns the one that is passed in
+    auto new_prop = prop->clone();
+    // set name and units if provided
+    if (!name.empty())
+      new_prop->setName(name);
+
+    if (!units.empty())
+      new_prop->setUnits(units);
+    self.addProperty(new_prop, replace);
     return;
   }
 

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

@@ -7,7 +7,7 @@
 import unittest
 import copy
 from mantid.geometry import Goniometer
-from mantid.kernel import DateAndTime, FloatTimeSeriesProperty, TimeROI
+from mantid.kernel import DateAndTime, FloatTimeSeriesProperty, TimeROI, FloatPropertyWithValue
 from mantid.api import Run
 import numpy as np
 
@@ -181,6 +181,68 @@ def test_equals(self):
         other.addProperty("pressure", 1, True)
         self.assertNotEqual(self._run, other)
 
+    def test_setting_property_from_property(self):
+        property = FloatPropertyWithValue("property", 42)
+        property.units = "meters"
+
+        run = Run()
+        self.assertEqual(len(run.getProperties()), 0)
+
+        # not setting name or units
+        run.addProperty("", property, True)
+        self.assertEqual(len(run.getProperties()), 1)
+        self.assertTrue("property" in run)
+        prop = run.getProperty("property")
+        self.assertEqual(prop.name, "property")
+        self.assertEqual(prop.value, 42)
+        self.assertEqual(prop.units, "meters")
+
+        # setting name but not units
+        run.addProperty("prop1", property, True)
+        self.assertEqual(len(run.getProperties()), 2)
+        self.assertTrue("prop1" in run)
+        prop = run.getProperty("prop1")
+        self.assertEqual(prop.name, "prop1")
+        self.assertEqual(prop.value, 42)
+        self.assertEqual(prop.units, "meters")
+
+        # setting name and units
+        run.addProperty("prop2", property, "yards", True)
+        self.assertEqual(len(run.getProperties()), 3)
+        self.assertTrue("prop2" in run)
+        prop = run.getProperty("prop2")
+        self.assertEqual(prop.name, "prop2")
+        self.assertEqual(prop.value, 42)
+        self.assertEqual(prop.units, "yards")
+
+        # test __setitem__
+        run["prop3"] = property
+        self.assertEqual(len(run.getProperties()), 4)
+        self.assertTrue("prop3" in run)
+        prop = run.getProperty("prop3")
+        self.assertEqual(prop.name, "prop3")
+        self.assertEqual(prop.value, 42)
+        self.assertEqual(prop.units, "meters")
+
+        # test with time series property
+        time_series = FloatTimeSeriesProperty("time_series")
+        time_series.addValue("2008-Jun-17 11:10:44", 1.0)
+        time_series.addValue("2008-Jun-17 11:10:45", 2.0)
+
+        run.addProperty("", time_series, True)
+        self.assertEqual(len(run.getProperties()), 5)
+        self.assertTrue("time_series" in run)
+        tsp = run.getProperty("time_series")
+        self.assertEqual(tsp.name, "time_series")
+        self.assertEqual(list(tsp.value), [1, 2])
+
+        run.addProperty("other_time_series", time_series, True)
+        self.assertEqual(len(run.getProperties()), 6)
+        self.assertTrue("other_time_series" in run)
+        tsp2 = run.getProperty("other_time_series")
+        self.assertEqual(tsp2.name, "other_time_series")
+        self.assertEqual(list(tsp2.value), [1, 2])
+
 
 if __name__ == "__main__":
     unittest.main()

buildconfig/CMake/CppCheck_Suppressions.txt.in

@@ -916,7 +916,7 @@ unknownMacro:${CMAKE_SOURCE_DIR}/Framework/PythonInterface/mantid/api/src/Export
 cstyleCast:${CMAKE_SOURCE_DIR}/Framework/PythonInterface/mantid/api/src/Exports/MDGeometry.cpp:96
 unknownMacro:${CMAKE_SOURCE_DIR}/Framework/PythonInterface/mantid/api/src/Exports/MatrixWorkspace.cpp:65
 cstyleCast:${CMAKE_SOURCE_DIR}/Framework/PythonInterface/mantid/api/src/Exports/MultipleExperimentInfos.cpp:36
-syntaxError:${CMAKE_SOURCE_DIR}/Framework/PythonInterface/mantid/api/src/Exports/Run.cpp:251
+syntaxError:${CMAKE_SOURCE_DIR}/Framework/PythonInterface/mantid/api/src/Exports/Run.cpp:258
 syntaxError:${CMAKE_SOURCE_DIR}/Framework/PythonInterface/mantid/api/src/Exports/Sample.cpp:81
 syntaxError:${CMAKE_SOURCE_DIR}/Framework/PythonInterface/mantid/api/src/Exports/SpectrumDefinition.cpp:40
 cstyleCast:${CMAKE_SOURCE_DIR}/Framework/PythonInterface/mantid/api/src/Exports/SpectrumInfo.cpp:87

docs/source/release/v6.13.0/Framework/Python/Bugfixes/38886.rst

@@ -0,0 +1 @@
+- Fixed a bug where the method :meth:`mantid.api.Run.addProperty` was ignoring the ``name`` and ``units`` parameters if the ``value`` was of type :class:`mantid.kernel.Property`. Now only if the ``name`` and ``units`` are empty will the existing values on the ``Property`` be used.