From 7a7dd5cb72b89dcf53604e412a75168f266f462b Mon Sep 17 00:00:00 2001 From: Mark Abraham Date: Sat, 25 Sep 2021 07:01:42 +0000 Subject: [PATCH] Improve ObservablesReducer and its handling --- src/gromacs/mdrun/md.cpp | 6 +++ src/gromacs/mdrun/mimic.cpp | 3 ++ src/gromacs/mdrun/minimize.cpp | 8 +++- src/gromacs/mdrun/rerun.cpp | 5 +++ src/gromacs/mdtypes/observablesreducer.cpp | 2 +- src/gromacs/mdtypes/observablesreducer.h | 14 +++++-- .../mdtypes/tests/observablesreducer.cpp | 15 +++---- .../computeglobalselement.cpp | 41 +++++++++++++++++-- 8 files changed, 78 insertions(+), 16 deletions(-) diff --git a/src/gromacs/mdrun/md.cpp b/src/gromacs/mdrun/md.cpp index 1e7479b304..3870f0fb92 100644 --- 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 82852d21bf..91b1d17940 100644 --- 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 27a7bf1fea..7513e11df3 100644 --- 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 d383df413b..48372cebfd 100644 --- 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 83db804cc1..255efa1d7c 100644 --- 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 4221f20940..4d65067232 100644 --- 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 f621949245..81debf1b22 100644 --- 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 97634447ba..4c3894dd6d 100644 --- a/src/gromacs/modularsimulator/computeglobalselement.cpp +++ b/src/gromacs/modularsimulator/computeglobalselement.cpp @@ -129,6 +129,8 @@ void ComputeGlobalsElement::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::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 @@ -339,6 +344,32 @@ ComputeGlobalsElement::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; @@ -355,7 +386,7 @@ ISimulatorElement* ComputeGlobalsElement::get GlobalCommunicationHelper* globalCommunicationHelper, ObservablesReducer* observablesReducer) { - auto* element = builderHelper->storeElement( + ComputeGlobalsElement* element = builderHelper->storeElement( std::make_unique>( statePropagatorData, energyData, @@ -373,6 +404,8 @@ ISimulatorElement* ComputeGlobalsElement::get legacySimulatorData->top_global, legacySimulatorData->constr, observablesReducer)); + builderHelper->registerPostStepScheduling( + registerPostStepSchedulingFunction(element->observablesReducer_)); return element; } @@ -395,11 +428,11 @@ ISimulatorElement* ComputeGlobalsElement(cachedValue.value()); + return std::any_cast(cachedValue.value()); } else { - ISimulatorElement* vvComputeGlobalsElement = builderHelper->storeElement( + ComputeGlobalsElement* vvComputeGlobalsElement = builderHelper->storeElement( std::make_unique>( statePropagatorData, energyData, @@ -418,6 +451,8 @@ ISimulatorElement* ComputeGlobalsElementconstr, observablesReducer)); builderHelper->storeBuilderData(key, vvComputeGlobalsElement); + builderHelper->registerPostStepScheduling( + registerPostStepSchedulingFunction(vvComputeGlobalsElement->observablesReducer_)); return vvComputeGlobalsElement; } } -- 2.22.0