Improve ObservablesReducer and its handling

diff --git a/src/gromacs/mdrun/md.cpp b/src/gromacs/mdrun/md.cpp
index 1e7479b3046a212d2e0bf806c8fd3a476b68b633..3870f0fb92f79cb7e524ec2536e6a672fd419676 100644 (file)
--- a/src/gromacs/mdrun/md.cpp
+++ b/src/gromacs/mdrun/md.cpp
@@ -668,6 +668,9 @@ void gmx::LegacySimulator::do_md()
                         cglo_flags_iteration,
                         step,
                         &observablesReducer);
+        // Clean up after pre-step use of compute_globals()
+        observablesReducer.markAsReadyToReduce();
+
         if (cglo_flags_iteration & CGLO_STOPCM)
         {
             /* At initialization, do not pass x with acceleration-correction mode
@@ -712,6 +715,8 @@ void gmx::LegacySimulator::do_md()
                         cglo_flags & ~CGLO_PRESSURE,
                         step,
                         &observablesReducer);
+        // Clean up after pre-step use of compute_globals()
+        observablesReducer.markAsReadyToReduce();
     }
 
     /* Calculate the initial half step temperature, and save the ekinh_old */
@@ -1990,6 +1995,7 @@ void gmx::LegacySimulator::do_md()
         /* increase the MD step number */
         step++;
         step_rel++;
+        observablesReducer.markAsReadyToReduce();
 
 #if GMX_FAHCORE
         if (MASTER(cr))

diff --git a/src/gromacs/mdrun/mimic.cpp b/src/gromacs/mdrun/mimic.cpp
index 82852d21bf41ee650ccc89fc7ffb8d39b82f3388..91b1d1794008c9266b834bd090cdbbf1e81dec41 100644 (file)
--- a/src/gromacs/mdrun/mimic.cpp
+++ b/src/gromacs/mdrun/mimic.cpp
@@ -364,6 +364,8 @@ void gmx::LegacySimulator::do_mimic()
                         cglo_flags,
                         step,
                         &observablesReducer);
+        // Clean up after pre-step use of compute_globals()
+        observablesReducer.markAsReadyToReduce();
     }
 
     if (MASTER(cr))
@@ -750,6 +752,7 @@ void gmx::LegacySimulator::do_mimic()
         /* increase the MD step number */
         step++;
         step_rel++;
+        observablesReducer.markAsReadyToReduce();
     }
     /* End of main MD loop */
 

diff --git a/src/gromacs/mdrun/minimize.cpp b/src/gromacs/mdrun/minimize.cpp
index 27a7bf1fea371909c42b0eb81dae30c99796c514..7513e11df3841b6734d8b536dbaf7c9baa7e60cc 100644 (file)
--- a/src/gromacs/mdrun/minimize.cpp
+++ b/src/gromacs/mdrun/minimize.cpp
@@ -1380,6 +1380,7 @@ void LegacySimulator::do_cg()
      * We do not unshift, so molecules are always whole in congrad.c
      */
     energyEvaluator.run(s_min, mu_tot, vir, pres, -1, TRUE, step);
+    observablesReducer.markAsReadyToReduce();
 
     if (MASTER(cr))
     {
@@ -1573,6 +1574,7 @@ void LegacySimulator::do_cg()
         neval++;
         /* Calculate energy for the trial step */
         energyEvaluator.run(s_c, mu_tot, vir, pres, -1, FALSE, step);
+        observablesReducer.markAsReadyToReduce();
 
         /* Calc derivative along line */
         const rvec*                    pc  = s_c->s.cg_p.rvec_array();
@@ -1688,6 +1690,7 @@ void LegacySimulator::do_cg()
                 neval++;
                 /* Calculate energy for the trial step */
                 energyEvaluator.run(s_b, mu_tot, vir, pres, -1, FALSE, step);
+                observablesReducer.markAsReadyToReduce();
 
                 /* p does not change within a step, but since the domain decomposition
                  * might change, we have to use cg_p of s_b here.
@@ -1880,7 +1883,7 @@ void LegacySimulator::do_cg()
          * If we have reached machine precision, converged is already set to true.
          */
         converged = converged || (s_min->fmax < inputrec->em_tol);
-
+        observablesReducer.markAsReadyToReduce();
     } /* End of the loop */
 
     if (converged)
@@ -2712,7 +2715,7 @@ void LegacySimulator::do_lbfgs()
          * If we have reached machine precision, converged is already set to true.
          */
         converged = converged || (ems.fmax < inputrec->em_tol);
-
+        observablesReducer.markAsReadyToReduce();
     } /* End of the loop */
 
     if (converged)
@@ -3072,6 +3075,7 @@ void LegacySimulator::do_steep()
         }
 
         count++;
+        observablesReducer.markAsReadyToReduce();
     } /* End of the loop  */
 
     /* Print some data...  */

diff --git a/src/gromacs/mdrun/rerun.cpp b/src/gromacs/mdrun/rerun.cpp
index d383df413b34d5ab258c89f42eb89c67ed73946d..48372cebfd7a687a6f29ff7238eed14cd19eea20 100644 (file)
--- a/src/gromacs/mdrun/rerun.cpp
+++ b/src/gromacs/mdrun/rerun.cpp
@@ -413,6 +413,8 @@ void gmx::LegacySimulator::do_rerun()
                         cglo_flags,
                         step,
                         &observablesReducer);
+        // Clean up after pre-step use of compute_globals()
+        observablesReducer.markAsReadyToReduce();
     }
 
     if (MASTER(cr))
@@ -760,6 +762,8 @@ void gmx::LegacySimulator::do_rerun()
                             cglo_flags,
                             step,
                             &observablesReducer);
+            // Clean up after pre-step use of compute_globals()
+            observablesReducer.markAsReadyToReduce();
         }
 
         /* Note: this is OK, but there are some numerical precision issues with using the convergence of
@@ -874,6 +878,7 @@ void gmx::LegacySimulator::do_rerun()
             step++;
             step_rel++;
         }
+        observablesReducer.markAsReadyToReduce();
     }
     /* End of main MD loop */
 

diff --git a/src/gromacs/mdtypes/observablesreducer.cpp b/src/gromacs/mdtypes/observablesreducer.cpp
index 83db804cc1b26f11e67c1c46a669d5bd2a342f4e..255efa1d7c2e5f384fe120a9337c579fccfca011 100644 (file)
--- a/src/gromacs/mdtypes/observablesreducer.cpp
+++ b/src/gromacs/mdtypes/observablesreducer.cpp
@@ -152,7 +152,7 @@ void ObservablesReducer::reductionComplete(Step step)
     impl_->reduceSoon_ = false;
 }
 
-void ObservablesReducer::stepComplete()
+void ObservablesReducer::markAsReadyToReduce()
 {
     impl_->status_ = ObservablesReducerStatus::ReadyToReduce;
 }

diff --git a/src/gromacs/mdtypes/observablesreducer.h b/src/gromacs/mdtypes/observablesreducer.h
index 4221f20940496a886aec9788e5f74a7fe173895b..4d65067232ff9b4735a3bde6eb1703fa1cff6402 100644 (file)
--- a/src/gromacs/mdtypes/observablesreducer.h
+++ b/src/gromacs/mdtypes/observablesreducer.h
@@ -148,6 +148,9 @@ moduleB =>> observablesReducer [label="returns"];
 observablesReducer =>> observablesReducer [label="zeroes reduction buffer"];
 observablesReducer =>> compute_globals [label="returns"];
 
+runner =>> observablesReducer [label="notifies at end of step"];
+
+
 \endmsc
  *
  * Three callbacks are produced and called per participating module:
@@ -440,7 +443,8 @@ public:
      * problematic output after reduction.
      */
     void reductionComplete(Step step);
-    /*! \brief Notify the ObservablesReducer that this MD step is complete
+    /*! \brief Notify the ObservablesReducer that it should make
+     * ready to receive new values to reduce
      *
      * Any runner using the ObservablesReducer must call this method
      * whenever a step completes, so that subscribed modules can use
@@ -452,8 +456,12 @@ public:
      * reduction it can notify them of that status. This permits them
      * to check their own requirements, e.g. that
      * ReductionRequirement::Soon will operate this step or next
-     * step. */
-    void stepComplete();
+     * step.
+     *
+     * For the same reason, it is also necessary to call this method
+     * at a suitable point after uses of an ObservablesReducer before
+     * the regular steps of a runner. */
+    void markAsReadyToReduce();
     //! The builder needs to be able to make the Impl object
     friend class ObservablesReducerBuilder;
 };

diff --git a/src/gromacs/mdtypes/tests/observablesreducer.cpp b/src/gromacs/mdtypes/tests/observablesreducer.cpp
index f621949245d4ec06c1ed2f05ef441710d3962b0f..81debf1b22a9f5fc373aa0acda1e459810c4322f 100644 (file)
--- a/src/gromacs/mdtypes/tests/observablesreducer.cpp
+++ b/src/gromacs/mdtypes/tests/observablesreducer.cpp
@@ -73,6 +73,7 @@ TEST(ObservablesReducerTest, CanMoveConstruct)
     ObservablesReducer        observablesReducer(std::move(observablesReducerOriginal));
     EXPECT_TRUE(observablesReducer.communicationBuffer().empty())
             << "no buffer available when no subscribers requested reduction";
+    observablesReducer.markAsReadyToReduce();
 }
 
 TEST(ObservablesReducerTest, CanBuildAndUseWithNoSubscribers)
@@ -86,7 +87,7 @@ TEST(ObservablesReducerTest, CanBuildAndUseWithNoSubscribers)
 
     EXPECT_TRUE(observablesReducer.communicationBuffer().empty())
             << "no buffer available after reductionComplete()";
-    observablesReducer.stepComplete();
+    observablesReducer.markAsReadyToReduce();
 }
 
 TEST(ObservablesReducerTest, CanBuildAndUseWithOneSubscriber)
@@ -138,7 +139,7 @@ TEST(ObservablesReducerTest, CanBuildAndUseWithOneSubscriber)
 
         // Note that there's nothing else to check here, because the
         // empty buffer means that no reduction should take place.
-        observablesReducer.stepComplete();
+        observablesReducer.markAsReadyToReduce();
     }
     {
         SCOPED_TRACE("Test that ReductionRequirement::Soon does trigger behavior");
@@ -161,7 +162,7 @@ TEST(ObservablesReducerTest, CanBuildAndUseWithOneSubscriber)
                 << "reduction step is passed through correctly";
         EXPECT_THAT(bufferView, testing::AllOf(testing::SizeIs(requiredBufferSize), testing::Each(0.0)))
                 << "buffer is zeroed after reduction";
-        observablesReducer.stepComplete();
+        observablesReducer.markAsReadyToReduce();
     }
 }
 
@@ -369,7 +370,7 @@ TEST_P(ObservablesReducerIntegrationTest, CanBuildAndUseSimply)
         rankData.observablesReducer.value().reductionComplete(step);
         EXPECT_TRUE(rankData.observablesReducer.value().communicationBuffer().empty())
                 << "no buffer available after reductionComplete()";
-        rankData.observablesReducer.value().stepComplete();
+        rankData.observablesReducer.value().markAsReadyToReduce();
     }
 }
 
@@ -405,7 +406,7 @@ TEST_P(ObservablesReducerIntegrationTest, CanBuildAndUseOverMultipleSteps)
             rankData.observablesReducer.value().reductionComplete(step);
             EXPECT_TRUE(rankData.observablesReducer.value().communicationBuffer().empty())
                     << "no buffer available after reductionComplete()";
-            rankData.observablesReducer.value().stepComplete();
+            rankData.observablesReducer.value().markAsReadyToReduce();
         }
     }
 }
@@ -460,7 +461,7 @@ TEST_P(ObservablesReducerIntegrationTest, CanBuildAndUseWithoutAllNeedingReducti
         rankData.observablesReducer.value().reductionComplete(step);
         EXPECT_TRUE(rankData.observablesReducer.value().communicationBuffer().empty())
                 << "no buffer available after reductionComplete()";
-        rankData.observablesReducer.value().stepComplete();
+        rankData.observablesReducer.value().markAsReadyToReduce();
     }
 }
 
@@ -522,7 +523,7 @@ TEST_P(ObservablesReducerIntegrationTest, CanBuildAndUseWhenASubscriberUsesEvent
         rankData.observablesReducer.value().reductionComplete(step);
         EXPECT_TRUE(rankData.observablesReducer.value().communicationBuffer().empty())
                 << "no buffer available after reductionComplete()";
-        rankData.observablesReducer.value().stepComplete();
+        rankData.observablesReducer.value().markAsReadyToReduce();
     }
 }
 

diff --git a/src/gromacs/modularsimulator/computeglobalselement.cpp b/src/gromacs/modularsimulator/computeglobalselement.cpp
index 97634447ba97c8b7f9346f3486dc5700504067f0..4c3894dd6dc2ececb8ea22121c0f9bc1f65ab5fe 100644 (file)
--- a/src/gromacs/modularsimulator/computeglobalselement.cpp
+++ b/src/gromacs/modularsimulator/computeglobalselement.cpp
@@ -129,6 +129,8 @@ void ComputeGlobalsElement<algorithm>::elementSetup()
         // to call compute_globals twice.
 
         compute(-1, CGLO_GSTAT | CGLO_STOPCM, nullSignaller_.get(), false, true);
+        // Clean up after pre-step use of compute()
+        observablesReducer_->markAsReadyToReduce();
 
         auto v = statePropagatorData_->velocitiesView();
         // At initialization, do not pass x with acceleration-correction mode
@@ -156,6 +158,9 @@ void ComputeGlobalsElement<algorithm>::elementSetup()
     {
         copy_mat(energyData_->ekindata()->tcstat[i].ekinh, energyData_->ekindata()->tcstat[i].ekinh_old);
     }
+
+    // Clean up after pre-step use of compute()
+    observablesReducer_->markAsReadyToReduce();
 }
 
 template<ComputeGlobalsAlgorithm algorithm>
@@ -339,6 +344,32 @@ ComputeGlobalsElement<algorithm>::registerTrajectorySignallerCallback(Trajectory
     return std::nullopt;
 }
 
+namespace
+{
+
+/*! \brief Schedule a function for actions that must happen at the end of each step
+ *
+ * After reduction, an ObservablesReducer is marked as unavailable for
+ * further reduction this step. This needs to be reset in order to be
+ * used on the next step.
+ *
+ * \param[in]  observablesReducer The ObservablesReducer to mark as ready for use
+ */
+SchedulingFunction registerPostStepSchedulingFunction(ObservablesReducer* observablesReducer)
+{
+    SchedulingFunction postStepSchedulingFunction =
+            [observablesReducer](
+                    Step /*step*/, Time /*time*/, const RegisterRunFunction& registerRunFunction) {
+                SimulatorRunFunction completeObservablesReducerStep = [&observablesReducer]() {
+                    observablesReducer->markAsReadyToReduce();
+                };
+                registerRunFunction(completeObservablesReducerStep);
+            };
+    return postStepSchedulingFunction;
+}
+
+} // namespace
+
 //! Explicit template instantiation
 //! \{
 template class ComputeGlobalsElement<ComputeGlobalsAlgorithm::LeapFrog>;
@@ -355,7 +386,7 @@ ISimulatorElement* ComputeGlobalsElement<ComputeGlobalsAlgorithm::LeapFrog>::get
         GlobalCommunicationHelper*              globalCommunicationHelper,
         ObservablesReducer*                     observablesReducer)
 {
-    auto* element = builderHelper->storeElement(
+    ComputeGlobalsElement* element = builderHelper->storeElement(
             std::make_unique<ComputeGlobalsElement<ComputeGlobalsAlgorithm::LeapFrog>>(
                     statePropagatorData,
                     energyData,
@@ -373,6 +404,8 @@ ISimulatorElement* ComputeGlobalsElement<ComputeGlobalsAlgorithm::LeapFrog>::get
                     legacySimulatorData->top_global,
                     legacySimulatorData->constr,
                     observablesReducer));
+    builderHelper->registerPostStepScheduling(
+            registerPostStepSchedulingFunction(element->observablesReducer_));
 
     return element;
 }
@@ -395,11 +428,11 @@ ISimulatorElement* ComputeGlobalsElement<ComputeGlobalsAlgorithm::VelocityVerlet
 
     if (cachedValue)
     {
-        return std::any_cast<ISimulatorElement*>(cachedValue.value());
+        return std::any_cast<ComputeGlobalsElement*>(cachedValue.value());
     }
     else
     {
-        ISimulatorElement* vvComputeGlobalsElement = builderHelper->storeElement(
+        ComputeGlobalsElement* vvComputeGlobalsElement = builderHelper->storeElement(
                 std::make_unique<ComputeGlobalsElement<ComputeGlobalsAlgorithm::VelocityVerlet>>(
                         statePropagatorData,
                         energyData,
@@ -418,6 +451,8 @@ ISimulatorElement* ComputeGlobalsElement<ComputeGlobalsAlgorithm::VelocityVerlet
                         simulator->constr,
                         observablesReducer));
         builderHelper->storeBuilderData(key, vvComputeGlobalsElement);
+        builderHelper->registerPostStepScheduling(
+                registerPostStepSchedulingFunction(vvComputeGlobalsElement->observablesReducer_));
         return vvComputeGlobalsElement;
     }
 }