Refactor simulator comparison tests
authorPascal Merz <pascal.merz@me.com>
Thu, 23 Jan 2020 14:00:09 +0000 (15:00 +0100)
committerPascal Merz <pascal.merz@me.com>
Tue, 23 Jun 2020 18:25:49 +0000 (18:25 +0000)
The rerun test and the simulator comparison test share some
infrastructure. The scope of shared functionality was chosen
a bit too broadly, making reuse for other similar tests difficult.
This change makes the shared functionality more granular, allowing
to reuse this functionality more broadly and increasing the
readability, at the expense of some minor code duplication.

src/programs/mdrun/tests/CMakeLists.txt
src/programs/mdrun/tests/rerun.cpp
src/programs/mdrun/tests/simulator.cpp
src/programs/mdrun/tests/simulatorcomparison.cpp
src/programs/mdrun/tests/simulatorcomparison.h

index 9b0322d78c5a2ae2d504452b3acf2c2201e1de7d..15fe0bd111d336b7051aa9604daf5d7fe6f2b9ed 100644 (file)
@@ -39,6 +39,7 @@ gmx_add_unit_test_library(mdrun_test_infrastructure
     energyreader.cpp
     energycomparison.cpp
     moduletest.cpp
+    simulatorcomparison.cpp
     terminationhelper.cpp
     trajectorycomparison.cpp
     trajectoryreader.cpp
index 15fc819e7d47bd4722373cd2f795792df8b1a673..b490d79901d05ae1bae88b9f44c48933165b66ed 100644 (file)
 #include "config.h"
 
 #include "gromacs/topology/ifunc.h"
-#include "gromacs/trajectory/energyframe.h"
 #include "gromacs/utility/stringutil.h"
 
+#include "testutils/mpitest.h"
+#include "testutils/simulationdatabase.h"
+
+#include "moduletest.h"
 #include "simulatorcomparison.h"
 
 namespace gmx
@@ -105,6 +108,58 @@ const TrajectoryFrameMatchSettings MdrunRerunTest::trajectoryMatchSettings = {
     ComparisonConditions::MustCompare
 };
 
+void executeRerunTest(TestFileManager*            fileManager,
+                      SimulationRunner*           runner,
+                      const std::string&          simulationName,
+                      int                         numWarningsToTolerate,
+                      const MdpFieldValues&       mdpFieldValues,
+                      const EnergyTermsToCompare& energyTermsToCompare,
+                      const TrajectoryComparison& trajectoryComparison)
+{
+    // TODO At some point we should also test PME-only ranks.
+    int numRanksAvailable = getNumberOfTestMpiRanks();
+    if (!isNumberOfPpRanksSupported(simulationName, numRanksAvailable))
+    {
+        fprintf(stdout,
+                "Test system '%s' cannot run with %d ranks.\n"
+                "The supported numbers are: %s\n",
+                simulationName.c_str(), numRanksAvailable,
+                reportNumbersOfPpRanksSupported(simulationName).c_str());
+        return;
+    }
+
+    // Set file names
+    auto simulator1TrajectoryFileName = fileManager->getTemporaryFilePath("sim1.trr");
+    auto simulator1EdrFileName        = fileManager->getTemporaryFilePath("sim1.edr");
+    auto simulator2TrajectoryFileName = fileManager->getTemporaryFilePath("sim2.trr");
+    auto simulator2EdrFileName        = fileManager->getTemporaryFilePath("sim2.edr");
+
+    // Run grompp
+    runner->tprFileName_ = fileManager->getTemporaryFilePath("sim.tpr");
+    runner->useTopGroAndNdxFromDatabase(simulationName);
+    runner->useStringAsMdpFile(prepareMdpFileContents(mdpFieldValues));
+    auto options = std::vector<SimulationOptionTuple>();
+    if (numWarningsToTolerate > 0)
+    {
+        options.emplace_back(SimulationOptionTuple("-maxwarn", std::to_string(numWarningsToTolerate)));
+    }
+    runGrompp(runner, options);
+
+    // Do first mdrun
+    runner->fullPrecisionTrajectoryFileName_ = simulator1TrajectoryFileName;
+    runner->edrFileName_                     = simulator1EdrFileName;
+    runMdrun(runner);
+
+    // Do second mdrun
+    runner->fullPrecisionTrajectoryFileName_ = simulator2TrajectoryFileName;
+    runner->edrFileName_                     = simulator2EdrFileName;
+    runMdrun(runner, { SimulationOptionTuple("-rerun", simulator1TrajectoryFileName) });
+
+    // Compare simulation results
+    compareEnergies(simulator1EdrFileName, simulator2EdrFileName, energyTermsToCompare);
+    compareTrajectories(simulator1TrajectoryFileName, simulator2TrajectoryFileName, trajectoryComparison);
+}
+
 TEST_P(MdrunRerunTest, WithinTolerances)
 {
     auto params         = GetParam();
index b1f9830931257e0285b34b34c88be63989064204..e6c3fa3188912d3c1ff977aec088efc24606833a 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2019, by the GROMACS development team, led by
+ * Copyright (c) 2019,2020, by the GROMACS development team, led by
  * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl,
  * and including many others, as listed in the AUTHORS file in the
  * top-level source directory and at http://www.gromacs.org.
 #include "config.h"
 
 #include "gromacs/topology/ifunc.h"
-#include "gromacs/trajectory/energyframe.h"
 #include "gromacs/utility/stringutil.h"
 
+#include "testutils/mpitest.h"
+#include "testutils/setenv.h"
+#include "testutils/simulationdatabase.h"
+
+#include "moduletest.h"
 #include "simulatorcomparison.h"
 
 namespace gmx
@@ -78,13 +82,25 @@ class SimulatorComparisonTest :
 
 TEST_P(SimulatorComparisonTest, WithinTolerances)
 {
-    auto params         = GetParam();
-    auto mdpParams      = std::get<0>(params);
-    auto simulationName = std::get<0>(mdpParams);
-    auto integrator     = std::get<1>(mdpParams);
-    auto tcoupling      = std::get<2>(mdpParams);
-    auto pcoupling      = std::get<3>(mdpParams);
-    auto envVariable    = std::get<1>(params);
+    auto params              = GetParam();
+    auto mdpParams           = std::get<0>(params);
+    auto simulationName      = std::get<0>(mdpParams);
+    auto integrator          = std::get<1>(mdpParams);
+    auto tcoupling           = std::get<2>(mdpParams);
+    auto pcoupling           = std::get<3>(mdpParams);
+    auto environmentVariable = std::get<1>(params);
+
+    // TODO At some point we should also test PME-only ranks.
+    int numRanksAvailable = getNumberOfTestMpiRanks();
+    if (!isNumberOfPpRanksSupported(simulationName, numRanksAvailable))
+    {
+        fprintf(stdout,
+                "Test system '%s' cannot run with %d ranks.\n"
+                "The supported numbers are: %s\n",
+                simulationName.c_str(), numRanksAvailable,
+                reportNumbersOfPpRanksSupported(simulationName).c_str());
+        return;
+    }
 
     if (integrator == "md-vv" && pcoupling == "Parrinello-Rahman")
     {
@@ -97,7 +113,7 @@ TEST_P(SimulatorComparisonTest, WithinTolerances)
             "Comparing two simulations of '%s' "
             "with integrator '%s' and '%s' temperature coupling, "
             "switching environment variable '%s'",
-            simulationName.c_str(), integrator.c_str(), tcoupling.c_str(), envVariable.c_str()));
+            simulationName.c_str(), integrator.c_str(), tcoupling.c_str(), environmentVariable.c_str()));
 
     auto mdpFieldValues = prepareMdpFieldValues(simulationName.c_str(), integrator.c_str(),
                                                 tcoupling.c_str(), pcoupling.c_str());
@@ -139,10 +155,51 @@ TEST_P(SimulatorComparisonTest, WithinTolerances)
     // trajectory frames in the chosen way.
     TrajectoryComparison trajectoryComparison{ trajectoryMatchSettings, trajectoryTolerances };
 
-    int numWarningsToTolerate = 0;
-    executeSimulatorComparisonTest(envVariable, &fileManager_, &runner_, simulationName,
-                                   numWarningsToTolerate, mdpFieldValues, energyTermsToCompare,
-                                   trajectoryComparison);
+    // Set file names
+    auto simulator1TrajectoryFileName = fileManager_.getTemporaryFilePath("sim1.trr");
+    auto simulator1EdrFileName        = fileManager_.getTemporaryFilePath("sim1.edr");
+    auto simulator2TrajectoryFileName = fileManager_.getTemporaryFilePath("sim2.trr");
+    auto simulator2EdrFileName        = fileManager_.getTemporaryFilePath("sim2.edr");
+
+    // Run grompp
+    runner_.tprFileName_ = fileManager_.getTemporaryFilePath("sim.tpr");
+    runner_.useTopGroAndNdxFromDatabase(simulationName);
+    runner_.useStringAsMdpFile(prepareMdpFileContents(mdpFieldValues));
+    runGrompp(&runner_);
+
+    // Backup current state of environment variable and unset it
+    char* environmentVariableBackup = getenv(environmentVariable.c_str());
+    gmxUnsetenv(environmentVariable.c_str());
+
+    // Do first mdrun
+    runner_.fullPrecisionTrajectoryFileName_ = simulator1TrajectoryFileName;
+    runner_.edrFileName_                     = simulator1EdrFileName;
+    runMdrun(&runner_);
+
+    // Set environment variable
+    const int overWriteEnvironmentVariable = 1;
+    gmxSetenv(environmentVariable.c_str(), "ON", overWriteEnvironmentVariable);
+
+    // Do second mdrun
+    runner_.fullPrecisionTrajectoryFileName_ = simulator2TrajectoryFileName;
+    runner_.edrFileName_                     = simulator2EdrFileName;
+    runMdrun(&runner_);
+
+    // Reset or unset environment variable to leave further tests undisturbed
+    if (environmentVariableBackup != nullptr)
+    {
+        // set environment variable
+        gmxSetenv(environmentVariable.c_str(), environmentVariableBackup, overWriteEnvironmentVariable);
+    }
+    else
+    {
+        // unset environment variable
+        gmxUnsetenv(environmentVariable.c_str());
+    }
+
+    // Compare simulation results
+    compareEnergies(simulator1EdrFileName, simulator2EdrFileName, energyTermsToCompare);
+    compareTrajectories(simulator1TrajectoryFileName, simulator2TrajectoryFileName, trajectoryComparison);
 }
 
 // TODO: The time for OpenCL kernel compilation means these tests time
index 816ce6fb6ba75ac318321c0e04a8e6033fa15f58..f79f2ae6111cb0c8426da4b1bc194f703ac22875 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2019, by the GROMACS development team, led by
+ * Copyright (c) 2019,2020, by the GROMACS development team, led by
  * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl,
  * and including many others, as listed in the AUTHORS file in the
  * top-level source directory and at http://www.gromacs.org.
@@ -34,7 +34,7 @@
  */
 /*! \internal \file
  * \brief
- * Helper classes for tests that compare the results of equivalent
+ * Helper functions for tests that compare the results of equivalent
  * simulation runs. Currently used for the rerun and the simulator
  * tests
  *
  */
 #include "gmxpre.h"
 
-#include "testutils/mpitest.h"
-#include "testutils/setenv.h"
-#include "testutils/simulationdatabase.h"
+#include "simulatorcomparison.h"
+
+#include "gromacs/trajectory/energyframe.h"
 
-#include "energycomparison.h"
 #include "energyreader.h"
 #include "mdruncomparison.h"
 #include "moduletest.h"
-#include "trajectorycomparison.h"
 #include "trajectoryreader.h"
 
 namespace gmx
 {
 namespace test
 {
-namespace
-{
 
-//! Run grompp and mdrun for both sets of mdp field values
-template<bool doEnvironmentVariable, bool doRerun>
-void executeSimulatorComparisonTestImpl(TestFileManager*            fileManager,
-                                        SimulationRunner*           runner,
-                                        const std::string&          simulationName,
-                                        int                         maxWarningsTolerated,
-                                        const MdpFieldValues&       mdpFieldValues,
-                                        const EnergyTermsToCompare& energyTermsToCompare,
-                                        const TrajectoryComparison& trajectoryComparison,
-                                        const std::string&          environmentVariable)
+void runGrompp(SimulationRunner* runner, const std::vector<SimulationOptionTuple>& options)
 {
-    // TODO At some point we should also test PME-only ranks.
-    int numRanksAvailable = getNumberOfTestMpiRanks();
-    if (!isNumberOfPpRanksSupported(simulationName, numRanksAvailable))
-    {
-        fprintf(stdout,
-                "Test system '%s' cannot run with %d ranks.\n"
-                "The supported numbers are: %s\n",
-                simulationName.c_str(), numRanksAvailable,
-                reportNumbersOfPpRanksSupported(simulationName).c_str());
-        return;
-    }
-
-    auto simulator1TrajectoryFileName = fileManager->getTemporaryFilePath("sim1.trr");
-    auto simulator1EdrFileName        = fileManager->getTemporaryFilePath("sim1.edr");
-    auto simulator2TrajectoryFileName = fileManager->getTemporaryFilePath("sim2.trr");
-    auto simulator2EdrFileName        = fileManager->getTemporaryFilePath("sim2.edr");
-    auto simulatorTprFileName         = fileManager->getTemporaryFilePath("sim.tpr");
+    CommandLine caller;
+    caller.append("grompp");
 
-    // prepare the .tpr file
+    for (const std::tuple<std::string, std::string>& option : options)
     {
-        // TODO evolve grompp to report the number of warnings issued, so
-        // tests always expect the right number.
-        CommandLine caller;
-        caller.append("grompp");
-        caller.addOption("-maxwarn", maxWarningsTolerated);
-        runner->tprFileName_ = simulatorTprFileName;
-        runner->useTopGroAndNdxFromDatabase(simulationName);
-        runner->useStringAsMdpFile(prepareMdpFileContents(mdpFieldValues));
-        EXPECT_EQ(0, runner->callGrompp(caller));
+        caller.addOption(std::get<0>(option).c_str(), std::get<1>(option));
     }
 
-    char* environmentVariableBackup = nullptr;
-    if (doEnvironmentVariable)
-    {
-        // save state of environment variable
-        environmentVariableBackup = getenv(environmentVariable.c_str());
-    }
+    EXPECT_EQ(0, runner->callGrompp(caller));
+}
 
-    // do the first mdrun
-    {
-        runner->fullPrecisionTrajectoryFileName_ = simulator1TrajectoryFileName;
-        runner->edrFileName_                     = simulator1EdrFileName;
-        runner->tprFileName_                     = simulatorTprFileName;
-        CommandLine simulator1Caller;
-        simulator1Caller.append("mdrun");
-        if (doEnvironmentVariable)
-        {
-            // unset environment variable
-            gmxUnsetenv(environmentVariable.c_str());
-        }
-        ASSERT_EQ(0, runner->callMdrun(simulator1Caller));
-    }
+void runMdrun(SimulationRunner* runner, const std::vector<SimulationOptionTuple>& options)
+{
+    CommandLine caller;
+    caller.append("mdrun");
 
-    // do the second mdrun
+    for (const std::tuple<std::string, std::string>& option : options)
     {
-        runner->fullPrecisionTrajectoryFileName_ = simulator2TrajectoryFileName;
-        runner->edrFileName_                     = simulator2EdrFileName;
-        runner->tprFileName_                     = simulatorTprFileName;
-        CommandLine simulator2Caller;
-        simulator2Caller.append("mdrun");
-        if (doEnvironmentVariable)
-        {
-            // set environment variable
-            gmxSetenv(environmentVariable.c_str(), "ON", true);
-        }
-        if (doRerun)
-        {
-            simulator2Caller.addOption("-rerun", simulator1TrajectoryFileName);
-        }
-        ASSERT_EQ(0, runner->callMdrun(simulator2Caller));
+        caller.addOption(std::get<0>(option).c_str(), std::get<1>(option));
     }
 
-    if (doEnvironmentVariable)
-    {
-        if (environmentVariableBackup != nullptr)
-        {
-            // set environment variable
-            gmxSetenv(environmentVariable.c_str(), environmentVariableBackup, true);
-        }
-        else
-        {
-            // unset environment variable
-            gmxUnsetenv(environmentVariable.c_str());
-        }
-    }
+    EXPECT_EQ(0, runner->callMdrun(caller));
+}
 
+void compareEnergies(const std::string&          edr1Name,
+                     const std::string&          edr2Name,
+                     const EnergyTermsToCompare& energyTermsToCompare)
+{
     // Build the functor that will compare energy frames on the chosen
     // energy terms.
     EnergyComparison energyComparison(energyTermsToCompare);
@@ -169,31 +98,23 @@ void executeSimulatorComparisonTestImpl(TestFileManager*            fileManager,
     // names), for convenience. In the future, use a range.
     auto                                namesOfEnergiesToMatch = energyComparison.getEnergyNames();
     FramePairManager<EnergyFrameReader> energyManager(
-            openEnergyFileToReadTerms(simulator1EdrFileName, namesOfEnergiesToMatch),
-            openEnergyFileToReadTerms(simulator2EdrFileName, namesOfEnergiesToMatch));
+            openEnergyFileToReadTerms(edr1Name, namesOfEnergiesToMatch),
+            openEnergyFileToReadTerms(edr2Name, namesOfEnergiesToMatch));
     // Compare the energy frames.
     energyManager.compareAllFramePairs<EnergyFrame>(energyComparison);
+}
 
+void compareTrajectories(const std::string&          trajectory1Name,
+                         const std::string&          trajectory2Name,
+                         const TrajectoryComparison& trajectoryComparison)
+{
     // Build the manager that will present matching pairs of frames to compare
     FramePairManager<TrajectoryFrameReader> trajectoryManager(
-            std::make_unique<TrajectoryFrameReader>(simulator1TrajectoryFileName),
-            std::make_unique<TrajectoryFrameReader>(simulator2TrajectoryFileName));
+            std::make_unique<TrajectoryFrameReader>(trajectory1Name),
+            std::make_unique<TrajectoryFrameReader>(trajectory2Name));
     // Compare the trajectory frames.
     trajectoryManager.compareAllFramePairs<TrajectoryFrame>(trajectoryComparison);
 }
-} // namespace
-
-template<typename... Args>
-void executeSimulatorComparisonTest(const std::string& environmentVariable, Args&&... args)
-{
-    executeSimulatorComparisonTestImpl<true, false>(std::forward<Args>(args)..., environmentVariable);
-}
-
-template<typename... Args>
-void executeRerunTest(Args&&... args)
-{
-    executeSimulatorComparisonTestImpl<false, true>(std::forward<Args>(args)..., "");
-}
 
 } // namespace test
 } // namespace gmx
index b0e7843d96d80f7425bf38eabe87cb165d63ce0f..b31566dff9237fff1e517d618f967b98873ecc9d 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * This file is part of the GROMACS molecular simulation package.
  *
- * Copyright (c) 2019, by the GROMACS development team, led by
+ * Copyright (c) 2019,2020, by the GROMACS development team, led by
  * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl,
  * and including many others, as listed in the AUTHORS file in the
  * top-level source directory and at http://www.gromacs.org.
 
 #include <string>
 
+#include "energycomparison.h"
+#include "trajectorycomparison.h"
+
 namespace gmx
 {
 namespace test
 {
-/*!
- * \brief Run and compare a simulator run with and without an environment variable
- *
- * Run grompp, and repeat mdrun with and without the environment variable set.
- * Compare energies (via EnergyComparator) and trajectories.
- */
-template<typename... Args>
-void executeSimulatorComparisonTest(const std::string& environmentVariable, Args&&... args);
+class SimulationRunner;
 
-/*!
- * \brief Run and compare a simulator run to its rerun
- *
- * Run grompp, run mdrun and rerun the resulting trajectory.
- * Compare energies (via EnergyComparator) and trajectories.
- */
-template<typename... Args>
-void executeRerunTest(Args&&... args);
+typedef std::tuple<std::string, std::string> SimulationOptionTuple;
+
+void runGrompp(SimulationRunner*                         runner,
+               const std::vector<SimulationOptionTuple>& options = std::vector<SimulationOptionTuple>());
+
+void runMdrun(SimulationRunner*                         runner,
+              const std::vector<SimulationOptionTuple>& options = std::vector<SimulationOptionTuple>());
+
+void compareEnergies(const std::string&          edr1Name,
+                     const std::string&          edr2Name,
+                     const EnergyTermsToCompare& energyTermsToCompare);
+
+void compareTrajectories(const std::string&          trajectory1Name,
+                         const std::string&          trajectory2Name,
+                         const TrajectoryComparison& trajectoryComparison);
 
 } // namespace test
 } // namespace gmx
 
-// Including this here avoid having to put everything in the header file,
-// or to explicitly declare the templates (which would render the parameter
-// pack useless)
-#include "simulatorcomparison.cpp"
-
 #endif // GMX_PROGRAMS_MDRUN_TESTS_SIMULATORCOMPARISON_H