Improve validation code efficiency and robustness

rbauststfc · rbauststfc · commit 536fa472a16a · 2024-08-07T15:44:20.000+01:00
diff --git a/Framework/Algorithms/src/PolarizationCorrections/PolarizationEfficienciesWildes.cpp b/Framework/Algorithms/src/PolarizationCorrections/PolarizationEfficienciesWildes.cpp
@@ -128,69 +128,88 @@ void PolarizationEfficienciesWildes::init() {
 }
 
 namespace {
-
-void validateMatchingBins(const Mantid::API::MatrixWorkspace_sptr &workspace,
-                          const Mantid::API::MatrixWorkspace_sptr &refWs, const std::string &propertyName,
-                          std::map<std::string, std::string> &problems) {
+bool hasMatchingBins(const Mantid::API::MatrixWorkspace_sptr &workspace, const Mantid::API::MatrixWorkspace_sptr &refWs,
+                     const std::string &propertyName, std::map<std::string, std::string> &problems) {
   if (!WorkspaceHelpers::matchingBins(*workspace, *refWs, true)) {
     problems[propertyName] = "All input workspaces must have the same X values.";
-    return;
+    return false;
   }
+
+  return true;
 }
 
-void validateInputWorkspace(const Mantid::API::MatrixWorkspace_sptr &workspace,
-                            const Mantid::API::MatrixWorkspace_sptr &refWs, const std::string &propertyName,
-                            std::map<std::string, std::string> &problems) {
+bool isValidInputWorkspace(const Mantid::API::MatrixWorkspace_sptr &workspace,
+                           const Mantid::API::MatrixWorkspace_sptr &refWs, const std::string &propertyName,
+                           std::map<std::string, std::string> &problems) {
   if (workspace == nullptr) {
     problems[propertyName] = "All input workspaces must be matrix workspaces.";
-    return;
+    return false;
   }
 
   Kernel::Unit_const_sptr unit = workspace->getAxis(0)->unit();
   if (unit->unitID() != "Wavelength") {
     problems[propertyName] = "All input workspaces must be in units of Wavelength.";
-    return;
+    return false;
   }
 
   if (workspace->getNumberHistograms() != 1) {
     problems[propertyName] = "All input workspaces must contain only a single spectrum.";
-    return;
+    return false;
   }
 
-  validateMatchingBins(workspace, refWs, propertyName, problems);
+  return hasMatchingBins(workspace, refWs, propertyName, problems);
 }
 
-void validateInputWSGroup(const Mantid::API::WorkspaceGroup_sptr &groupWs, const std::string &propertyName,
-                          std::map<std::string, std::string> &problems) {
+bool isValidInputWSGroup(const Mantid::API::WorkspaceGroup_sptr &groupWs, const std::string &propertyName,
+                         std::map<std::string, std::string> &problems) {
   if (groupWs == nullptr) {
     problems[propertyName] = "The input workspace must be a group workspace.";
-    return;
+    return false;
   }
 
   if (groupWs->size() != 4) {
     problems[propertyName] = "The input group must contain a workspace for all four flipper configurations.";
-    return;
+    return false;
   }
 
   const MatrixWorkspace_sptr refWs = std::dynamic_pointer_cast<MatrixWorkspace>(groupWs->getItem(0));
   for (size_t i = 0; i < groupWs->size(); ++i) {
     const MatrixWorkspace_sptr childWs = std::dynamic_pointer_cast<MatrixWorkspace>(groupWs->getItem(i));
-    validateInputWorkspace(childWs, refWs, propertyName, problems);
+    if (!isValidInputWorkspace(childWs, refWs, propertyName, problems)) {
+      return false;
+    }
   }
+
+  return true;
 }
 } // namespace
 
 std::map<std::string, std::string> PolarizationEfficienciesWildes::validateInputs() {
   std::map<std::string, std::string> problems;
 
-  const WorkspaceGroup_sptr nonMagWsGrp = getProperty(PropNames::INPUT_NON_MAG_WS);
-  validateInputWSGroup(nonMagWsGrp, PropNames::INPUT_NON_MAG_WS, problems);
-  const MatrixWorkspace_sptr nonMagRefWs = std::dynamic_pointer_cast<MatrixWorkspace>(nonMagWsGrp->getItem(0));
-
   const bool hasMagWsGrp = !isDefault(PropNames::INPUT_MAG_WS);
   const bool hasInputPWs = !isDefault(PropNames::INPUT_P_EFF_WS);
   const bool hasInputAWs = !isDefault(PropNames::INPUT_A_EFF_WS);
 
+  if (!isDefault(PropNames::OUTPUT_P_EFF_WS) && !hasMagWsGrp && !hasInputPWs && !hasInputAWs) {
+    problems[PropNames::OUTPUT_P_EFF_WS] = "If output polarizer efficiency is requested then either the magnetic "
+                                           "workspace or the known analyser efficiency should be provided.";
+  }
+
+  if (!isDefault(PropNames::OUTPUT_A_EFF_WS) && !hasMagWsGrp && !hasInputPWs && !hasInputAWs) {
+    problems[PropNames::OUTPUT_A_EFF_WS] = "If output analyser efficiency is requested then either the magnetic "
+                                           "workspace or the known polarizer efficiency should be provided.";
+  }
+
+  const WorkspaceGroup_sptr nonMagWsGrp = getProperty(PropNames::INPUT_NON_MAG_WS);
+  if (!isValidInputWSGroup(nonMagWsGrp, PropNames::INPUT_NON_MAG_WS, problems)) {
+    // We need to use a child workspace from the nonMag group as a reference for later checks, so stop here if there are
+    // any issues with this input
+    return problems;
+  }
+
+  const MatrixWorkspace_sptr nonMagRefWs = std::dynamic_pointer_cast<MatrixWorkspace>(nonMagWsGrp->getItem(0));
+
   // If a magnetic workspace has been provided then we will use that to calculate the polarizer and analyser
   // efficiencies, so individual efficiency workspaces should not be provided as well
   if (hasMagWsGrp) {
@@ -203,34 +222,23 @@ std::map<std::string, std::string> PolarizationEfficienciesWildes::validateInput
     }
 
     const WorkspaceGroup_sptr magWsGrp = getProperty(PropNames::INPUT_MAG_WS);
-    validateInputWSGroup(magWsGrp, PropNames::INPUT_MAG_WS, problems);
-
-    if (problems.find(PropNames::INPUT_MAG_WS) == problems.end()) {
+    if (isValidInputWSGroup(magWsGrp, PropNames::INPUT_MAG_WS, problems)) {
+      // Check that bins match between the input mag and nonMag workspace groups
       const MatrixWorkspace_sptr magWs = std::dynamic_pointer_cast<MatrixWorkspace>(magWsGrp->getItem(0));
-      validateMatchingBins(magWs, nonMagRefWs, PropNames::INPUT_MAG_WS, problems);
+      hasMatchingBins(magWs, nonMagRefWs, PropNames::INPUT_MAG_WS, problems);
     }
   } else {
     if (hasInputPWs) {
       const MatrixWorkspace_sptr inputPolEffWs = getProperty(PropNames::INPUT_P_EFF_WS);
-      validateInputWorkspace(inputPolEffWs, nonMagRefWs, PropNames::INPUT_P_EFF_WS, problems);
+      isValidInputWorkspace(inputPolEffWs, nonMagRefWs, PropNames::INPUT_P_EFF_WS, problems);
     }
 
     if (hasInputAWs) {
       const MatrixWorkspace_sptr inputAnaEffWs = getProperty(PropNames::INPUT_A_EFF_WS);
-      validateInputWorkspace(inputAnaEffWs, nonMagRefWs, PropNames::INPUT_A_EFF_WS, problems);
+      isValidInputWorkspace(inputAnaEffWs, nonMagRefWs, PropNames::INPUT_A_EFF_WS, problems);
     }
   }
 
-  if (!isDefault(PropNames::OUTPUT_P_EFF_WS) && !hasMagWsGrp && !hasInputPWs && !hasInputAWs) {
-    problems[PropNames::OUTPUT_P_EFF_WS] = "If output polarizer efficiency is requested then either the magnetic "
-                                           "workspace or the known analyser efficiency should be provided.";
-  }
-
-  if (!isDefault(PropNames::OUTPUT_A_EFF_WS) && !hasMagWsGrp && !hasInputPWs && !hasInputAWs) {
-    problems[PropNames::OUTPUT_A_EFF_WS] = "If output analyser efficiency is requested then either the magnetic "
-                                           "workspace or the known polarizer efficiency should be provided.";
-  }
-
   return problems;
 }
 
diff --git a/Framework/Algorithms/test/PolarizationCorrections/PolarizationEfficienciesWildesTest.h b/Framework/Algorithms/test/PolarizationCorrections/PolarizationEfficienciesWildesTest.h
@@ -10,6 +10,7 @@
 
 #include "MantidAPI/AnalysisDataService.h"
 #include "MantidAPI/MatrixWorkspace.h"
+#include "MantidAPI/WorkspaceFactory.h"
 #include "MantidAPI/WorkspaceGroup.h"
 #include "MantidAlgorithms/CreateSampleWorkspace.h"
 #include "MantidAlgorithms/GroupWorkspaces.h"
@@ -21,6 +22,7 @@ using Mantid::Algorithms::GroupWorkspaces;
 using Mantid::Algorithms::PolarizationEfficienciesWildes;
 using Mantid::API::AnalysisDataService;
 using Mantid::API::MatrixWorkspace_sptr;
+using Mantid::API::WorkspaceFactory;
 using Mantid::API::WorkspaceGroup_sptr;
 using Mantid::DataObjects::TableWorkspace;
 
@@ -241,6 +243,20 @@ class PolarizationEfficienciesWildesTest : public CxxTest::TestSuite {
     assertSetPropertyThrowsInvalidArgumentError<TableWorkspace>(InputPropNames::A_EFF_WS, invalidWsType);
   }
 
+  void test_input_non_mag_not_matrix_ws_group_throws_error() {
+    const auto nonMagGrp = createTableWorkspaceGrp("nonMagWs");
+    const auto magGrp = createMagWSGroup("magWs");
+    const auto alg = createEfficiencyAlg(nonMagGrp, magGrp);
+    assertValidationError(alg, InputPropNames::NON_MAG_WS, PropErrors::WS_GRP_CHILD_TYPE_ERROR);
+  }
+
+  void test_input_mag_not_matrix_ws_group_throws_error() {
+    const auto nonMagGrp = createNonMagWSGroup("nonMagWs");
+    const auto magGrp = createTableWorkspaceGrp("magWs");
+    const auto alg = createEfficiencyAlg(nonMagGrp, magGrp);
+    assertValidationError(alg, InputPropNames::MAG_WS, PropErrors::WS_GRP_CHILD_TYPE_ERROR);
+  }
+
   /// Validation tests - valid property combinations
 
   void test_providing_both_mag_and_input_polarizer_efficiency_ws_throws_error() {
@@ -459,6 +475,23 @@ class PolarizationEfficienciesWildesTest : public CxxTest::TestSuite {
     return alg.getProperty("OutputWorkspace");
   }
 
+  WorkspaceGroup_sptr createTableWorkspaceGrp(const std::string &outName) {
+    const std::vector<std::string> &wsNames{outName + "_00", outName + "_01", outName + "_10", outName + "_11"};
+    for (const auto &wsName : wsNames) {
+      const auto ws = WorkspaceFactory::Instance().createTable();
+      AnalysisDataService::Instance().addOrReplace(wsName, ws);
+    }
+
+    GroupWorkspaces groupAlg;
+    groupAlg.initialize();
+    groupAlg.setChild(true);
+    groupAlg.setProperty("InputWorkspaces", wsNames);
+    groupAlg.setPropertyValue("OutputWorkspace", outName);
+    groupAlg.execute();
+
+    return groupAlg.getProperty("OutputWorkspace");
+  }
+
   std::unique_ptr<PolarizationEfficienciesWildes> createEfficiencyAlg(const WorkspaceGroup_sptr &nonMagWSGroup,
                                                                       const WorkspaceGroup_sptr &magWsGroup = nullptr) {
     auto alg = std::make_unique<PolarizationEfficienciesWildes>();
