fix name conflict loading

Author: andy-bridger

Framework/API/src/ExperimentInfo.cpp

@@ -367,7 +367,10 @@ void ExperimentInfo::populateInstrumentParameters() {
   for (const auto &item : paramInfoFromIDF) {
     const auto &nameComp = item.first;
     const auto &paramInfo = item.second;
-    const std::string &paramN = nameComp.first;
+    // Use the parameter's short name (e.g. "Alpha0"), not the cache key. The cache key may be
+    // function-qualified (e.g. "IkedaCarpenterPV:Alpha0") to keep two functions on the same
+    // component from clobbering each other, but downstream consumers always look up by short name.
+    const std::string &paramN = paramInfo->m_paramName;
 
     try {
       // Special case where user has specified r-position,t-position, and/or
@@ -1310,7 +1313,11 @@ void ExperimentInfo::populateWithParameter(Geometry::ParameterMap &paramMap,
         << paramInfo.m_constraint[0] << " , " << paramInfo.m_constraint[1] << " , " << paramInfo.m_penaltyFactor
         << " , " << paramInfo.m_tie << " , " << paramInfo.m_formula << " , " << paramInfo.m_formulaUnit << " , "
         << paramInfo.m_resultUnit << " , " << (*(paramInfo.m_interpolation));
-    paramMap.add("fitting", paramInfo.m_component, name, str.str(), pDescription, pVisible);
+    // Dedupe by (name, fitting function) — otherwise two functions on the same component sharing
+    // a parameter short name (e.g. Bk2BkExpConvPV:Gamma and IkedaCarpenterPV:Gamma) would clobber
+    // each other in the map.
+    paramMap.addFittingParameter(paramInfo.m_component, name, paramInfo.m_fittingFunction, str.str(), pDescription,
+                                 pVisible);
   } else if (category == "string") {
     paramMap.addString(paramInfo.m_component, name, paramInfo.m_value, pDescription, pVisible);
   } else if (category == "bool") {

Framework/API/src/IFunction.cpp

@@ -1231,7 +1231,15 @@ void IFunction::setMatrixWorkspace(std::shared_ptr<const API::MatrixWorkspace> w
 
     for (size_t i = 0; i < nParams(); i++) {
       if (!isExplicitlySet(i)) {
-        Geometry::Parameter_sptr param = paramMap.getRecursive(detectorPtr, parameterName(i), "fitting");
+        // Use the fitting-function-aware lookup: two functions on the same component can declare a
+        // parameter with the same short name (e.g. Bk2BkExpConvPV:Gamma and IkedaCarpenterPV:Gamma).
+        // The plain getRecursive() short-circuits on the first match and would pick the wrong one.
+        Geometry::Parameter_sptr param =
+            paramMap.getRecursiveFittingParameter(detectorPtr, parameterName(i), this->name());
+        if (!param) {
+          // Fall back for IDFs that omit the function prefix (no embedded function name to match).
+          param = paramMap.getRecursive(detectorPtr, parameterName(i), "fitting");
+        }
         if (param != Geometry::Parameter_sptr()) {
           // get FitParameter
           const auto &fitParam = param->value<Geometry::FitParameter>();

Framework/API/test/ExperimentInfoTest.h

@@ -14,8 +14,11 @@
 #include "MantidGeometry/Instrument/Detector.h"
 #include "MantidGeometry/Instrument/DetectorGroup.h"
 #include "MantidGeometry/Instrument/DetectorInfo.h"
+#include "MantidGeometry/Instrument/FitParameter.h"
+#include "MantidGeometry/Instrument/XMLInstrumentParameter.h"
 #include "MantidKernel/ConfigService.h"
 #include "MantidKernel/DateAndTime.h"
+#include "MantidKernel/Interpolation.h"
 #include "MantidKernel/Matrix.h"
 #include "MantidKernel/SingletonHolder.h"
 
@@ -703,6 +706,58 @@ class ExperimentInfoTest : public CxxTest::TestSuite {
     TS_ASSERT_EQUALS(value, "11;22;33;44");
   }
 
+  void test_populateInstrumentParameters_handles_two_fitting_functions_sharing_parameter_name() {
+    // Regression test for the IDF parse bug: two functions on the same component declaring the
+    // same parameter short name (e.g. Bk2BkExpConvPV:Gamma and IkedaCarpenterPV:Gamma) used to
+    // collide in the IDF cache; the fix function-qualifies the cache key, while the runtime
+    // ParameterMap still stores entries under the short name "Gamma".
+    Instrument_sptr inst = ComponentCreationHelper::createTestInstrumentCylindrical(1);
+    ExperimentInfo expt;
+    expt.setInstrument(inst);
+
+    const auto comp = inst->getChild(0).get();
+    auto makeXMLParam = [&](const std::string &fittingFunction, const std::string &value) {
+      auto interp = std::make_shared<Kernel::Interpolation>();
+      std::string penaltyFactor;
+      // populateWithParameter indexes m_constraint at [0] and [1]; mirror the IDF parser, which
+      // always passes a 2-element vector.
+      std::vector<std::string> constraint(2, "");
+      return std::make_shared<XMLInstrumentParameter>(
+          /*logfileID*/ "", /*value*/ value, interp, /*formula*/ "", /*formulaUnit*/ "", /*resultUnit*/ "",
+          /*paramName*/ "Gamma", /*type*/ "fitting", /*tie*/ "", constraint, penaltyFactor,
+          /*fitFunc*/ fittingFunction, /*extractSingleValueAs*/ "", /*eq*/ "", comp, /*angleConvertConst*/ 1.0,
+          /*description*/ "", /*visible*/ "true");
+    };
+
+    // Insert into the cache under function-qualified keys, mirroring what InstrumentDefinitionParser
+    // now does.
+    auto &cache = inst->getLogfileCache();
+    cache.insert({{"Bk2BkExpConvPV:Gamma", comp}, makeXMLParam("Bk2BkExpConvPV", "1.5")});
+    cache.insert({{"IkedaCarpenterPV:Gamma", comp}, makeXMLParam("IkedaCarpenterPV", "2.5")});
+
+    expt.populateInstrumentParameters();
+
+    const auto &paramMap = expt.instrumentParameters();
+
+    // Both entries must be present in the runtime ParameterMap.
+    auto bk2bk = paramMap.getRecursiveFittingParameter(comp, "Gamma", "Bk2BkExpConvPV");
+    TS_ASSERT(bk2bk);
+    TS_ASSERT_EQUALS(bk2bk->value<FitParameter>().getFunction(), "Bk2BkExpConvPV");
+    TS_ASSERT_DELTA(bk2bk->value<FitParameter>().getValue(), 1.5, 1e-12);
+
+    auto ikedaPV = paramMap.getRecursiveFittingParameter(comp, "Gamma", "IkedaCarpenterPV");
+    TS_ASSERT(ikedaPV);
+    TS_ASSERT_EQUALS(ikedaPV->value<FitParameter>().getFunction(), "IkedaCarpenterPV");
+    TS_ASSERT_DELTA(ikedaPV->value<FitParameter>().getValue(), 2.5, 1e-12);
+
+    // Backwards-compat: short-name lookup must still find a fitting parameter (callers like
+    // NormaliseByDetector that don't supply a function name should keep working).
+    auto byShortName = paramMap.getRecursive(comp, "Gamma", "fitting");
+    TS_ASSERT(byShortName);
+    const auto &fp = byShortName->value<FitParameter>();
+    TS_ASSERT(fp.getFunction() == "Bk2BkExpConvPV" || fp.getFunction() == "IkedaCarpenterPV");
+  }
+
 private:
   void addInstrumentWithParameter(ExperimentInfo &expt, const std::string &name, const std::string &value) {
     Instrument_sptr inst = ComponentCreationHelper::createTestInstrumentCylindrical(1);

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

@@ -131,6 +131,13 @@ class MANTID_GEOMETRY_DLL ParameterMap {
   void add(const IComponent *comp, const std::shared_ptr<Parameter> &par,
            const std::string *const pDescription = nullptr);
 
+  /// Add a fitting parameter. Dedupes by (name, fittingFunction) rather than just name so that two
+  /// functions attached to the same component sharing a parameter name (e.g. IkedaCarpenterPV:Alpha0
+  /// and IkedaCarpenterMD:Alpha0) can coexist in the map.
+  void addFittingParameter(const IComponent *comp, const std::string &name, const std::string &fittingFunction,
+                           const std::string &value, const std::string *const pDescription = nullptr,
+                           const std::string &pVisible = "true");
+
   /** @name Helper methods for adding and updating parameter types  */
   /// Create or adjust "pos" parameter for a component
   void addPositionCoordinate(const IComponent *comp, const std::string &name, const double value,
@@ -195,6 +202,12 @@ class MANTID_GEOMETRY_DLL ParameterMap {
   /// a parameter with a specified type.
   std::shared_ptr<Parameter> getRecursiveByType(const IComponent *comp, const std::string &type) const;
 
+  /// Look for a fitting parameter recursively, picking the one whose embedded FitParameter function name
+  /// matches fittingFunction. Multiple fitting parameters can share a short name on the same component
+  /// (one per function) — getRecursive() would short-circuit on the first match, this walks them all.
+  std::shared_ptr<Parameter> getRecursiveFittingParameter(const IComponent *comp, const std::string &name,
+                                                          const std::string &fittingFunction) const;
+
   /** Get the values of a given parameter of all the components that have the
    * name: compName
    *  @tparam The parameter type

Framework/Geometry/src/Instrument/InstrumentDefinitionParser.cpp

@@ -2365,7 +2365,12 @@ void InstrumentDefinitionParser::setLogfile(const Geometry::IComponent *comp, co
       description = pDescription->getAttribute("is");
     }
 
-    auto cacheKey = std::make_pair(paramName, comp);
+    // Qualify the cache key with the fitting-function name (when present) so that two functions
+    // attached to the same component sharing a parameter name (e.g. IkedaCarpenterPV:Alpha0 and
+    // IkedaCarpenterMD:Alpha0) do not overwrite each other. The stored XMLInstrumentParameter
+    // still keeps the unqualified paramName so downstream lookups by short name continue to work.
+    const std::string cacheParamKey = fittingFunction.empty() ? paramName : fittingFunction + ":" + paramName;
+    auto cacheKey = std::make_pair(cacheParamKey, comp);
     auto cacheValue = std::make_shared<XMLInstrumentParameter>(
         logfileID, value, interpolation, formula, formulaUnit, resultUnit, paramName, type, tie, constraint,
         penaltyFactor, fittingFunction, extractSingleValueAs, eq, comp, m_angleConvertConst, description, visible);

Framework/Geometry/src/Instrument/ParameterMap.cpp

@@ -9,6 +9,7 @@
 #include "MantidGeometry/Instrument.h"
 #include "MantidGeometry/Instrument/ComponentInfo.h"
 #include "MantidGeometry/Instrument/DetectorInfo.h"
+#include "MantidGeometry/Instrument/FitParameter.h"
 #include "MantidGeometry/Instrument/ParComponentFactory.h"
 #include "MantidGeometry/Instrument/ParameterFactory.h"
 #include "MantidKernel/Cache.h"
@@ -389,6 +390,55 @@ void ParameterMap::add(const IComponent *comp, const std::shared_ptr<Parameter>
   }
 }
 
+/** Add a fitting parameter, deduplicating by (name, fittingFunction) instead of name alone.
+ *
+ * Two different fitting functions attached to the same component can declare a parameter with
+ * the same short name (e.g. Bk2BkExpConvPV:Gamma and IkedaCarpenterPV:Gamma). The regular add()
+ * path uses positionOf() which only matches by name and would let one entry silently overwrite
+ * the other.
+ *
+ * @param comp :: Component the parameter is attached to
+ * @param name :: Parameter short name (e.g. "Gamma")
+ * @param fittingFunction :: Name of the fitting function this parameter applies to (may be empty)
+ * @param value :: Serialised FitParameter value string (as built by ExperimentInfo)
+ * @param pDescription :: Optional description
+ * @param pVisible :: Visibility flag
+ */
+void ParameterMap::addFittingParameter(const IComponent *comp, const std::string &name,
+                                       const std::string &fittingFunction, const std::string &value,
+                                       const std::string *const pDescription, const std::string &pVisible) {
+  checkIsNotMaskingParameter(name);
+  auto param = ParameterFactory::create("fitting", name, pVisible);
+  param->fromString(value);
+  if (pDescription)
+    param->setDescription(*pDescription);
+
+  // Look for an existing fitting parameter on this component with the same name AND the same
+  // embedded function; only that one should be replaced.
+  if (!m_map.empty()) {
+    const ComponentID id = comp->getComponentID();
+    auto it_found = m_map.find(id);
+    if (it_found != m_map.end()) {
+      auto itrs = m_map.equal_range(id);
+      for (auto itr = itrs.first; itr != itrs.second; ++itr) {
+        const auto &existing = itr->second;
+        if (existing->type() == "fitting" && strcasecmp(existing->nameAsCString(), name.c_str()) == 0) {
+          try {
+            if (existing->value<FitParameter>().getFunction() == fittingFunction) {
+              std::atomic_store(&(itr->second), param);
+              return;
+            }
+          } catch (...) {
+            // Not a FitParameter value despite the type tag, fall through to insert.
+          }
+        }
+      }
+    }
+  }
+
+  m_map.insert({comp->getComponentID(), param});
+}
+
 /** Create or adjust "pos" parameter for a component
  * Assumed that name either equals "x", "y" or "z" otherwise this
  * method will not add or modify "pos" parameter
@@ -881,6 +931,49 @@ Parameter_sptr ParameterMap::getRecursive(const IComponent *comp, const char *na
   return result;
 }
 
+/** Find a fitting parameter recursively, picking the one whose embedded FitParameter function name
+ * matches the requested fittingFunction.
+ *
+ * When two functions attached to the same component share a parameter short name
+ * (e.g. Bk2BkExpConvPV:Gamma and IkedaCarpenterPV:Gamma), getRecursive() short-circuits on the first
+ * match and may return the wrong function's parameter. This method walks every entry on each level
+ * of the component tree so it can disambiguate by function name.
+ *
+ * @param comp :: The component to start the search with
+ * @param name :: Parameter short name (e.g. "Gamma")
+ * @param fittingFunction :: The fitting function name this parameter must belong to
+ * @returns the matching parameter, or a null shared pointer if none is found
+ */
+Parameter_sptr ParameterMap::getRecursiveFittingParameter(const IComponent *comp, const std::string &name,
+                                                          const std::string &fittingFunction) const {
+  checkIsNotMaskingParameter(name);
+  if (!comp || m_map.empty())
+    return Parameter_sptr();
+
+  std::shared_ptr<const IComponent> compInFocus(comp, NoDeleting());
+  while (compInFocus != nullptr) {
+    const ComponentID id = compInFocus->getComponentID();
+    auto it_found = m_map.find(id);
+    if (it_found != m_map.end()) {
+      auto itrs = m_map.equal_range(id);
+      for (auto itr = itrs.first; itr != itrs.second; ++itr) {
+        const auto &param = itr->second;
+        if (param->type() == "fitting" && strcasecmp(param->nameAsCString(), name.c_str()) == 0) {
+          try {
+            if (param->value<FitParameter>().getFunction() == fittingFunction) {
+              return std::atomic_load(&itr->second);
+            }
+          } catch (...) {
+            // Not a FitParameter value despite the type tag, keep looking.
+          }
+        }
+      }
+    }
+    compInFocus = compInFocus->getParent();
+  }
+  return Parameter_sptr();
+}
+
 /**
  * Return the value of a parameter as a string
  * @param comp :: Component to which parameter is related

Framework/Geometry/test/ParameterMapTest.h

@@ -10,6 +10,7 @@
 #include "MantidBeamline/DetectorInfo.h"
 #include "MantidFrameworkTestHelpers/ComponentCreationHelper.h"
 #include "MantidGeometry/Instrument/Detector.h"
+#include "MantidGeometry/Instrument/FitParameter.h"
 #include "MantidGeometry/Instrument/Parameter.h"
 #include "MantidGeometry/Instrument/ParameterFactory.h"
 #include "MantidGeometry/Instrument/ParameterMap.h"
@@ -609,6 +610,80 @@ class ParameterMapTest : public CxxTest::TestSuite {
     TS_ASSERT_EQUALS(pmap.get(comp, "v")->asString(), "[0.123456789012345,0.123456789012345,0.123456789012345]");
   }
 
+  void test_addFittingParameter_keeps_entries_for_different_functions() {
+    // Two functions on the same component declare a parameter with the same short name
+    // (e.g. Bk2BkExpConvPV:Gamma and IkedaCarpenterPV:Gamma). addFittingParameter must
+    // dedupe by (name, function), so both entries should coexist.
+    ParameterMap pmap;
+    IComponent_sptr comp = m_testInstrument->getChild(0);
+    pmap.addFittingParameter(comp.get(), "Gamma", "Bk2BkExpConvPV", "1.5 , Bk2BkExpConvPV , Gamma");
+    pmap.addFittingParameter(comp.get(), "Gamma", "IkedaCarpenterPV", "2.5 , IkedaCarpenterPV , Gamma");
+
+    TS_ASSERT_EQUALS(pmap.size(), 2);
+
+    auto bk2bk = pmap.getRecursiveFittingParameter(comp.get(), "Gamma", "Bk2BkExpConvPV");
+    TS_ASSERT(bk2bk);
+    TS_ASSERT_EQUALS(bk2bk->value<Mantid::Geometry::FitParameter>().getFunction(), "Bk2BkExpConvPV");
+    TS_ASSERT_DELTA(bk2bk->value<Mantid::Geometry::FitParameter>().getValue(), 1.5, 1e-12);
+
+    auto ikedaPV = pmap.getRecursiveFittingParameter(comp.get(), "Gamma", "IkedaCarpenterPV");
+    TS_ASSERT(ikedaPV);
+    TS_ASSERT_EQUALS(ikedaPV->value<Mantid::Geometry::FitParameter>().getFunction(), "IkedaCarpenterPV");
+    TS_ASSERT_DELTA(ikedaPV->value<Mantid::Geometry::FitParameter>().getValue(), 2.5, 1e-12);
+
+    // Lookup for a function that was never registered yields nothing.
+    TS_ASSERT(!pmap.getRecursiveFittingParameter(comp.get(), "Gamma", "IkedaCarpenterMD"));
+  }
+
+  void test_addFittingParameter_replaces_only_same_function_entry() {
+    // A second add for the SAME (name, function) must replace, not duplicate.
+    ParameterMap pmap;
+    IComponent_sptr comp = m_testInstrument->getChild(0);
+    pmap.addFittingParameter(comp.get(), "Gamma", "Bk2BkExpConvPV", "1.5 , Bk2BkExpConvPV , Gamma");
+    pmap.addFittingParameter(comp.get(), "Gamma", "IkedaCarpenterPV", "2.5 , IkedaCarpenterPV , Gamma");
+
+    pmap.addFittingParameter(comp.get(), "Gamma", "Bk2BkExpConvPV", "9.9 , Bk2BkExpConvPV , Gamma");
+
+    // Still two entries — the IkedaCarpenterPV one is untouched, Bk2BkExpConvPV one is overwritten.
+    TS_ASSERT_EQUALS(pmap.size(), 2);
+
+    auto bk2bk = pmap.getRecursiveFittingParameter(comp.get(), "Gamma", "Bk2BkExpConvPV");
+    TS_ASSERT(bk2bk);
+    TS_ASSERT_DELTA(bk2bk->value<Mantid::Geometry::FitParameter>().getValue(), 9.9, 1e-12);
+
+    auto ikedaPV = pmap.getRecursiveFittingParameter(comp.get(), "Gamma", "IkedaCarpenterPV");
+    TS_ASSERT(ikedaPV);
+    TS_ASSERT_DELTA(ikedaPV->value<Mantid::Geometry::FitParameter>().getValue(), 2.5, 1e-12);
+  }
+
+  void test_getRecursiveFittingParameter_walks_up_component_tree() {
+    // Parameter declared on the parent should be visible from a child component.
+    ParameterMap pmap;
+    IComponent_sptr parent = m_testInstrument; // the instrument itself
+    IComponent_sptr child = m_testInstrument->getChild(0);
+    pmap.addFittingParameter(parent.get(), "Gamma", "IkedaCarpenterPV", "3.3 , IkedaCarpenterPV , Gamma");
+
+    auto found = pmap.getRecursiveFittingParameter(child.get(), "Gamma", "IkedaCarpenterPV");
+    TS_ASSERT(found);
+    TS_ASSERT_DELTA(found->value<Mantid::Geometry::FitParameter>().getValue(), 3.3, 1e-12);
+  }
+
+  void test_getRecursive_still_finds_one_entry_after_addFittingParameter() {
+    // Backwards-compat: callers that don't care which function the param belongs to should
+    // still get a hit from the plain getRecursive("Gamma", "fitting") path.
+    ParameterMap pmap;
+    IComponent_sptr comp = m_testInstrument->getChild(0);
+    pmap.addFittingParameter(comp.get(), "Gamma", "Bk2BkExpConvPV", "1.5 , Bk2BkExpConvPV , Gamma");
+    pmap.addFittingParameter(comp.get(), "Gamma", "IkedaCarpenterPV", "2.5 , IkedaCarpenterPV , Gamma");
+
+    auto found = pmap.getRecursive(comp.get(), "Gamma", "fitting");
+    TS_ASSERT(found);
+    // Both entries have type "fitting" with name "Gamma", so getRecursive returns whichever the
+    // multimap iterates first — we don't assert which, but we assert the FitParameter is valid.
+    const auto &fp = found->value<Mantid::Geometry::FitParameter>();
+    TS_ASSERT(fp.getFunction() == "Bk2BkExpConvPV" || fp.getFunction() == "IkedaCarpenterPV");
+  }
+
 private:
   template <typename ValueType>
   void doCopyAndUpdateTestUsingGenericAdd(const std::string &type, const ValueType &origValue,