Fix #3655

This fixes #3655 by addressing some readability issues in
velocityscalingtemperaturecoupling.cpp, and adding some trivial
const qualifiers in the simulator tests.

Closes #3655

diff --git a/src/gromacs/modularsimulator/simulatoralgorithm.h b/src/gromacs/modularsimulator/simulatoralgorithm.h
index ab4146bfa556f89d028ae5d8406043ec49183a1d..8073f0055f02d157bf4921394dbda6c8902367f7 100644 (file)
--- a/src/gromacs/modularsimulator/simulatoralgorithm.h
+++ b/src/gromacs/modularsimulator/simulatoralgorithm.h
@@ -81,6 +81,11 @@ class TopologyHolder;
  * runs the simulation by, in turn, building a task list from the elements
  * for a predefined number of steps, then running the task list, and repeating
  * until the stop criterion is fulfilled.
+ *
+ * The simulator algorithm is owning all elements involved in the simulation
+ * and is hence controlling their lifetime. This ensures that pointers and
+ * callbacks exchanged between elements remain valid throughout the duration
+ * of the simulation run.
  */
 class ModularSimulatorAlgorithm final
 {

diff --git a/src/gromacs/modularsimulator/velocityscalingtemperaturecoupling.cpp b/src/gromacs/modularsimulator/velocityscalingtemperaturecoupling.cpp
index e6633898272e50ee12cd518909387746b84be9db..1cc09b0b6ec1233799c9a3d94d4d18946d0274b1 100644 (file)
--- a/src/gromacs/modularsimulator/velocityscalingtemperaturecoupling.cpp
+++ b/src/gromacs/modularsimulator/velocityscalingtemperaturecoupling.cpp
@@ -437,11 +437,16 @@ const std::string& VelocityScalingTemperatureCoupling::clientID()
 
 real VelocityScalingTemperatureCoupling::conservedEnergyContribution() const
 {
-    return (reportPreviousConservedEnergy_ == ReportPreviousStepConservedEnergy::Yes)
-                   ? std::accumulate(temperatureCouplingIntegralPreviousStep_.begin(),
-                                     temperatureCouplingIntegralPreviousStep_.end(), 0.0)
-                   : std::accumulate(temperatureCouplingIntegral_.begin(),
-                                     temperatureCouplingIntegral_.end(), 0.0);
+    if (reportPreviousConservedEnergy_ == ReportPreviousStepConservedEnergy::Yes)
+    {
+        return std::accumulate(temperatureCouplingIntegralPreviousStep_.begin(),
+                               temperatureCouplingIntegralPreviousStep_.end(), 0.0);
+    }
+    else
+    {
+        return std::accumulate(temperatureCouplingIntegral_.begin(),
+                               temperatureCouplingIntegral_.end(), 0.0);
+    }
 }
 
 ISimulatorElement* VelocityScalingTemperatureCoupling::getElementPointerImpl(
@@ -455,6 +460,7 @@ ISimulatorElement* VelocityScalingTemperatureCoupling::getElementPointerImpl(
         UseFullStepKE                         useFullStepKE,
         ReportPreviousStepConservedEnergy     reportPreviousStepConservedEnergy)
 {
+    // Element is now owned by the caller of this method, who will handle lifetime (see ModularSimulatorAlgorithm)
     auto* element = builderHelper->storeElement(std::make_unique<VelocityScalingTemperatureCoupling>(
             legacySimulatorData->inputrec->nsttcouple, offset, useFullStepKE, reportPreviousStepConservedEnergy,
             legacySimulatorData->inputrec->ld_seed, legacySimulatorData->inputrec->opts.ngtc,
@@ -462,6 +468,7 @@ ISimulatorElement* VelocityScalingTemperatureCoupling::getElementPointerImpl(
             legacySimulatorData->inputrec->opts.ref_t, legacySimulatorData->inputrec->opts.tau_t,
             legacySimulatorData->inputrec->opts.nrdf, energyData, legacySimulatorData->inputrec->etc));
     auto* thermostat = static_cast<VelocityScalingTemperatureCoupling*>(element);
+    // Capturing pointer is safe because lifetime is handled by caller
     builderHelper->registerThermostat([thermostat](const PropagatorThermostatConnection& connection) {
         thermostat->connectWithPropagator(connection);
     });

diff --git a/src/programs/mdrun/tests/simulator.cpp b/src/programs/mdrun/tests/simulator.cpp
index 222b4ae41d02c9638ba64995ad81cc53e490d854..31be90d878ebe8ed8cc7a2039986f9e614e1b600 100644 (file)
--- a/src/programs/mdrun/tests/simulator.cpp
+++ b/src/programs/mdrun/tests/simulator.cpp
@@ -82,16 +82,16 @@ 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 environmentVariable = std::get<1>(params);
+    const auto& params              = GetParam();
+    const auto& mdpParams           = std::get<0>(params);
+    const auto& simulationName      = std::get<0>(mdpParams);
+    const auto& integrator          = std::get<1>(mdpParams);
+    const auto& tcoupling           = std::get<2>(mdpParams);
+    const auto& pcoupling           = std::get<3>(mdpParams);
+    const auto& environmentVariable = std::get<1>(params);
 
     // TODO At some point we should also test PME-only ranks.
-    int numRanksAvailable = getNumberOfTestMpiRanks();
+    const int numRanksAvailable = getNumberOfTestMpiRanks();
     if (!isNumberOfPpRanksSupported(simulationName, numRanksAvailable))
     {
         fprintf(stdout,
@@ -109,7 +109,7 @@ TEST_P(SimulatorComparisonTest, WithinTolerances)
         return;
     }
 
-    auto hasConservedField = !(tcoupling == "no" && pcoupling == "no");
+    const auto hasConservedField = !(tcoupling == "no" && pcoupling == "no");
 
     SCOPED_TRACE(formatString(
             "Comparing two simulations of '%s' "
@@ -118,8 +118,8 @@ TEST_P(SimulatorComparisonTest, WithinTolerances)
             simulationName.c_str(), integrator.c_str(), tcoupling.c_str(), pcoupling.c_str(),
             environmentVariable.c_str()));
 
-    auto mdpFieldValues = prepareMdpFieldValues(simulationName.c_str(), integrator.c_str(),
-                                                tcoupling.c_str(), pcoupling.c_str());
+    const auto mdpFieldValues = prepareMdpFieldValues(simulationName.c_str(), integrator.c_str(),
+                                                      tcoupling.c_str(), pcoupling.c_str());
 
     EnergyTermsToCompare energyTermsToCompare{ {
             { interaction_function[F_EPOT].longname, relativeToleranceAsPrecisionDependentUlp(10.0, 100, 80) },
@@ -152,13 +152,13 @@ TEST_P(SimulatorComparisonTest, WithinTolerances)
     }
 
     // Specify how trajectory frame matching must work.
-    TrajectoryFrameMatchSettings trajectoryMatchSettings{ true,
-                                                          true,
-                                                          true,
-                                                          ComparisonConditions::MustCompare,
-                                                          ComparisonConditions::MustCompare,
-                                                          ComparisonConditions::MustCompare,
-                                                          MaxNumFrames::compareAllFrames() };
+    const TrajectoryFrameMatchSettings trajectoryMatchSettings{ true,
+                                                                true,
+                                                                true,
+                                                                ComparisonConditions::MustCompare,
+                                                                ComparisonConditions::MustCompare,
+                                                                ComparisonConditions::MustCompare,
+                                                                MaxNumFrames::compareAllFrames() };
     TrajectoryTolerances trajectoryTolerances = TrajectoryComparison::s_defaultTrajectoryTolerances;
     if (simulationName != "argon12")
     {
@@ -167,13 +167,13 @@ TEST_P(SimulatorComparisonTest, WithinTolerances)
 
     // Build the functor that will compare reference and test
     // trajectory frames in the chosen way.
-    TrajectoryComparison trajectoryComparison{ trajectoryMatchSettings, trajectoryTolerances };
+    const TrajectoryComparison trajectoryComparison{ trajectoryMatchSettings, trajectoryTolerances };
 
     // 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");
+    const auto simulator1TrajectoryFileName = fileManager_.getTemporaryFilePath("sim1.trr");
+    const auto simulator1EdrFileName        = fileManager_.getTemporaryFilePath("sim1.edr");
+    const auto simulator2TrajectoryFileName = fileManager_.getTemporaryFilePath("sim2.trr");
+    const auto simulator2EdrFileName        = fileManager_.getTemporaryFilePath("sim2.edr");
 
     // Run grompp
     runner_.tprFileName_ = fileManager_.getTemporaryFilePath("sim.tpr");
@@ -182,7 +182,7 @@ TEST_P(SimulatorComparisonTest, WithinTolerances)
     runGrompp(&runner_);
 
     // Backup current state of environment variable and unset it
-    char* environmentVariableBackup = getenv(environmentVariable.c_str());
+    const char* environmentVariableBackup = getenv(environmentVariable.c_str());
     gmxUnsetenv(environmentVariable.c_str());
 
     // Do first mdrun