Updates after review comments

Framework/API/inc/MantidAPI/AlgorithmManager.h

@@ -48,7 +48,6 @@ public:
                                                const int &version = -1) const;
 
   std::size_t size() const;
-  void setMaxAlgorithms(int n);
 
   IAlgorithm_sptr getAlgorithm(AlgorithmID id) const;
   void removeById(AlgorithmID id);
@@ -81,8 +80,6 @@ private:
   /// Removes any finished algorithms from the list of managed algorithms
   size_t removeFinishedAlgorithms();
 
-  /// The maximum size of the algorithm store
-  int m_max_no_algs;
   /// The list of managed algorithms
   std::deque<IAlgorithm_sptr>
       m_managed_algs; ///<  pointers to managed algorithms [policy???]

Framework/API/src/AlgorithmManager.cpp

@@ -22,15 +22,6 @@ Kernel::Logger g_log("AlgorithmManager");
 
 /// Private Constructor for singleton class
 AlgorithmManagerImpl::AlgorithmManagerImpl() : m_managed_algs() {
-  auto max_no_algs =
-      Kernel::ConfigService::Instance().getValue<int>("algorithms.retained");
-
-  m_max_no_algs = max_no_algs.get_value_or(0);
-
-  if (m_max_no_algs < 1) {
-    m_max_no_algs = 100; // Default to keeping 100 algorithms if not specified
-  }
-
   g_log.debug() << "Algorithm Manager created.\n";
 }
 
@@ -87,39 +78,10 @@ IAlgorithm_sptr AlgorithmManagerImpl::create(const std::string &algName,
         << count
         << " Finished algorithms removed from the managed algorithms list. "
         << m_managed_algs.size() << " remaining.\n";
-    // If this takes us beyond the maximum size, then remove the oldest one(s)
-    while (m_managed_algs.size() >=
-           static_cast<std::deque<IAlgorithm_sptr>::size_type>(m_max_no_algs)) {
-      std::deque<IAlgorithm_sptr>::iterator it;
-      it = m_managed_algs.begin();
-
-      // Look for the first (oldest) algo that is NOT running right now.
-      while (it != m_managed_algs.end()) {
-        if (!(*it)->isRunning())
-          break;
-        ++it;
-      }
-
-      if (it == m_managed_algs.end()) {
-        // Unusual case where ALL algorithms are running
-        g_log.warning()
-            << "All algorithms in the AlgorithmManager are running. "
-            << "Cannot pop oldest algorithm. "
-            << "You should increase your 'algorithms.retained' value. "
-            << m_managed_algs.size() << " in queue.\n";
-        break;
-      } else {
-        // Normal; erase that algorithm
-        g_log.debug() << "Popping out oldest algorithm " << (*it)->name()
-                      << '\n';
-        m_managed_algs.erase(it);
-      }
-    }
 
     // Add to list of managed ones
     m_managed_algs.emplace_back(alg);
     alg->initialize();
-
   } catch (std::runtime_error &ex) {
     g_log.error() << "AlgorithmManager:: Unable to create algorithm " << algName
                   << ' ' << ex.what() << '\n';
@@ -145,19 +107,6 @@ void AlgorithmManagerImpl::clear() {
 
 std::size_t AlgorithmManagerImpl::size() const { return m_managed_algs.size(); }
 
-/**
- * Set new maximum number of algorithms that can be stored.
- *
- * @param n :: The new maximum.
- */
-void AlgorithmManagerImpl::setMaxAlgorithms(int n) {
-  if (n < 0) {
-    throw std::runtime_error("Maximum number of algorithms stored in "
-                             "AlgorithmManager cannot be negative.");
-  }
-  m_max_no_algs = n;
-}
-
 /**
  * Returns a shared pointer by algorithm id
  * @param id :: The ID of the algorithm
@@ -250,22 +199,15 @@ void AlgorithmManagerImpl::cancelAll() {
 /// this does not lock the mutex as the locking is already assumed to be in
 /// place
 size_t AlgorithmManagerImpl::removeFinishedAlgorithms() {
-  std::vector<IAlgorithm_const_sptr> theCompletedInstances;
-  std::copy_if(
-      m_managed_algs.cbegin(), m_managed_algs.cend(),
-      std::back_inserter(theCompletedInstances), [](const auto &algorithm) {
+  size_t sizeBefore = m_managed_algs.size();
+
+  m_managed_algs.erase(std::remove_if(
+      m_managed_algs.begin(), m_managed_algs.end(), [](const auto &algorithm) {
         return (algorithm->executionState() == ExecutionState::Finished);
-      });
-  for (auto completedAlg : theCompletedInstances) {
-    auto itend = m_managed_algs.end();
-    for (auto it = m_managed_algs.begin(); it != itend; ++it) {
-      if ((**it).getAlgorithmID() == completedAlg->getAlgorithmID()) {
-        m_managed_algs.erase(it);
-        break;
-      }
-    }
-  }
-  return theCompletedInstances.size();
+      }));
+  size_t sizeAfter = m_managed_algs.size();
+
+  return sizeBefore - sizeAfter;
 }
 
 void AlgorithmManagerImpl::shutdown() { clear(); }

Framework/API/test/AlgorithmManagerTest.h

@@ -106,12 +106,6 @@ public:
   }
   static void destroySuite(AlgorithmManagerTest *suite) { delete suite; }
 
-  AlgorithmManagerTest() {
-    // A test fails unless algorithms.retained is big enough
-    Mantid::Kernel::ConfigService::Instance().setString("algorithms.retained",
-                                                        "5");
-  }
-
   void testVersionFail() {
     const size_t nalgs = AlgorithmFactory::Instance().getKeys().size();
     TS_ASSERT_THROWS(AlgorithmFactory::Instance().subscribe<AlgTestFail>(),
@@ -217,33 +211,8 @@ public:
     AlgorithmManager::Instance().notificationCenter.removeObserver(my_observer);
   }
 
-  /** Keep the right number of algorithms in the list.
-   *  This also tests setMaxAlgorithms().
-   */
-  void testDroppingOldOnes() {
-    AlgorithmManager::Instance().setMaxAlgorithms(5);
-    AlgorithmManager::Instance().clear();
-    TS_ASSERT_EQUALS(AlgorithmManager::Instance().size(), 0);
-
-    IAlgorithm_sptr first = AlgorithmManager::Instance().create("AlgTest");
-    // Fill up the list
-    for (size_t i = 1; i < 5; i++)
-      AlgorithmManager::Instance().create("AlgTest");
-    TS_ASSERT_EQUALS(AlgorithmManager::Instance().size(), 5);
-
-    // The first one is still in the list
-    TS_ASSERT(AlgorithmManager::Instance().getAlgorithm(
-                  first->getAlgorithmID()) == first);
-
-    // Add one more, drops the oldest one
-    AlgorithmManager::Instance().create("AlgTest");
-    TS_ASSERT_EQUALS(AlgorithmManager::Instance().size(), 5);
-    TS_ASSERT(
-        !AlgorithmManager::Instance().getAlgorithm(first->getAlgorithmID()));
-  }
-
-  /** Keep one algorithm running, drop the second-oldest one etc. */
-  void testDroppingOldOnes_whenAnAlgorithmIsStillRunning() {
+  /** Keep one algorithm running, run another to completion etc. */
+  void testDroppingCompletedOnes_whenAnAlgorithmIsStillRunning() {
     AlgorithmManager::Instance().clear();
     TS_ASSERT_EQUALS(AlgorithmManager::Instance().size(), 0);
 
@@ -294,20 +263,6 @@ public:
     AlgorithmManager::Instance().cancelAll();
   }
 
-  void testDroppingOldOnes_extremeCase() {
-    /** Extreme case where your queue fills up and all algos are running */
-    AlgorithmManager::Instance().clear();
-    for (size_t i = 0; i < 5; i++) {
-      AlgorithmManager::Instance().create("AlgRunsForever");
-    }
-
-    TS_ASSERT_EQUALS(AlgorithmManager::Instance().size(), 5);
-    // Create another that takes it past the normal max size (of 5)
-    AlgorithmManager::Instance().create("AlgTest");
-    TS_ASSERT_EQUALS(AlgorithmManager::Instance().size(), 6);
-    AlgorithmManager::Instance().cancelAll();
-  }
-
   void testThreadSafety() {
     PARALLEL_FOR_NO_WSP_CHECK()
     for (int i = 0; i < 5000; i++) {
@@ -317,7 +272,6 @@ public:
 
   void testRemovingByIdRemovesCorrectObject() {
     auto &mgr = AlgorithmManager::Instance();
-    mgr.setMaxAlgorithms(10);
     const size_t initialManagerSize = mgr.size();
     // 2 different ids for same named algorithm
     auto alg1 = mgr.create("AlgTest");
@@ -329,8 +283,6 @@ public:
     // the right one?
     auto foundAlg = mgr.getAlgorithm(alg2->getAlgorithmID());
     TS_ASSERT(foundAlg);
-
-    mgr.setMaxAlgorithms(5);
   }
 
   void test_runningInstancesOf() {

Framework/Kernel/src/ConfigService.cpp

@@ -651,8 +651,6 @@ void ConfigServiceImpl::createUserPropertiesFile() const {
     filestr << "##\n";
     filestr << "## GENERAL\n";
     filestr << "##\n\n";
-    filestr << "## Set the number of algorithm properties to retain\n";
-    filestr << "#algorithms.retained=90\n\n";
     filestr
         << "## Set the maximum number of cores used to run algorithms over\n";
     filestr << "#MultiThreaded.MaxCores=4\n\n";

Framework/Kernel/test/ConfigPropertyObserverTest.h

@@ -41,8 +41,6 @@ public:
         ConfigService::Instance().getString("datasearch.directories");
     m_defaultSaveDirectory =
         ConfigService::Instance().getString("defaultsave.directory");
-    m_retainedAlgorithms =
-        ConfigService::Instance().getString("algorithms.retained");
   }
 
   void tearDown() override {
@@ -50,8 +48,6 @@ public:
                                         m_searchDirectories);
     ConfigService::Instance().setString("defaultsave.directory",
                                         m_defaultSaveDirectory);
-    ConfigService::Instance().setString("algorithms.retained",
-                                        m_retainedAlgorithms);
   }
 
   void testRecievesCallbackForSearchDirectoryChange() {
@@ -96,7 +92,7 @@ public:
                          });
     auto callCountB = 0;
     auto observerB =
-        makeMockObserver("algorithms.retained",
+        makeMockObserver("defaultsave.directory",
                          [&callCountB](const std::string &newValue,
                                        const std::string &prevValue) -> void {
                            UNUSED_ARG(newValue);
@@ -105,7 +101,7 @@ public:
                          });
 
     ConfigService::Instance().setString("datasearch.directories", "/dev/null");
-    ConfigService::Instance().setString("algorithms.retained", "40");
+    ConfigService::Instance().setString("defaultsave.directory", "/dev/null");
 
     TS_ASSERT_EQUALS(1, callCountA);
     TS_ASSERT_EQUALS(1, callCountB);

Framework/Kernel/test/ConfigServiceTest.h

@@ -329,21 +329,20 @@ public:
 
   void TestCustomProperty() {
     std::string countString =
-        ConfigService::Instance().getString("algorithms.retained");
-    TS_ASSERT_EQUALS(countString, "50");
+        ConfigService::Instance().getString("projectRecovery.secondsBetween");
+    TS_ASSERT_EQUALS(countString, "60");
   }
 
   void TestCustomPropertyAsValue() {
-    // Mantid.legs is defined in the properties script as 6
     int value = ConfigService::Instance()
-                    .getValue<int>("algorithms.retained")
+                    .getValue<int>("projectRecovery.secondsBetween")
                     .get_value_or(0);
     double dblValue = ConfigService::Instance()
-                          .getValue<double>("algorithms.retained")
+                          .getValue<double>("projectRecovery.secondsBetween")
                           .get_value_or(0);
 
-    TS_ASSERT_EQUALS(value, 50);
-    TS_ASSERT_EQUALS(dblValue, 50.0);
+    TS_ASSERT_EQUALS(value, 60);
+    TS_ASSERT_EQUALS(dblValue, 60.0);
   }
 
   void TestMissingProperty() {

Framework/Properties/Mantid.properties.template

@@ -111,9 +111,6 @@ icatDownload.directory =
 # ICat mount point. Directory where archive is mounted. See Facility.xml filelocation.
 icatDownload.mountPoint =
 
-# The Number of algorithms properties to retain im memory for refence in scripts.
-algorithms.retained = 50
-
 # Defines the maximum number of cores to use for OpenMP
 # For machine default set to 0
 MultiThreaded.MaxCores = 0

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

@@ -109,9 +109,6 @@ void export_AlgorithmManager() {
                                      "Creates an unmanaged algorithm."))
       .def("size", &AlgorithmManagerImpl::size, arg("self"),
            "Returns the number of managed algorithms")
-      .def("setMaxAlgorithms", &AlgorithmManagerImpl::setMaxAlgorithms,
-           (arg("self"), arg("n")),
-           "Set the maximum number of allowed managed algorithms")
       .def("getAlgorithm", &getAlgorithm, (arg("self"), arg("id_holder")),
            "Return the algorithm instance identified by the given id.")
       .def("removeById", &removeById, (arg("self"), arg("id_holder")),

Framework/PythonInterface/test/python/plugins/algorithms/WorkflowAlgorithms/SimpleShapeMonteCarloAbsorptionTest.py

@@ -15,14 +15,14 @@ import unittest
 class SimpleShapeMonteCarloAbsorptionTest(unittest.TestCase):
     @classmethod
     def setUpClass(self):
-        __red_ws = Load('irs26176_graphite002_red.nxs')
-        __red_ws = ConvertUnits(
-            InputWorkspace=__red_ws,
+        red_ws = Load('irs26176_graphite002_red.nxs')
+        red_ws = ConvertUnits(
+            InputWorkspace=red_ws,
             Target='Wavelength',
             EMode='Indirect',
             EFixed=1.845)
 
-        self._red_ws = __red_ws
+        self._red_ws = red_ws
 
         self._arguments = {'ChemicalFormula': 'H2-O',
                            'DensityType': 'Mass Density',

docs/source/concepts/PropertiesFile.rst

@@ -36,9 +36,6 @@ General properties
 | ``algorithms.categories.hidden`` | A comma separated list of any categories of      | ``Muons,Testing``      |
 |                                  | algorithms that should be hidden in Mantid.      |                        |
 +----------------------------------+--------------------------------------------------+------------------------+
-| ``algorithms.retained``          | The Number of algorithms properties to retain in | ``50``                 |
-|                                  | memory for reference in scripts.                 |                        |
-+----------------------------------+--------------------------------------------------+------------------------+
 | ``curvefitting.guiExclude``      | A semicolon separated list of function names     | ``ExpDecay;Gaussian;`` |
 |                                  | that should be hidden in Mantid.                 |                        |
 +----------------------------------+--------------------------------------------------+------------------------+