Fix log writing (modular simulator)

The current implementation would not write energies to log if
- log was not in sync with energy writing, or
- log writing is more frequent than neighborsearching.

This commit fixes these bugs, restoring the legacy behavior. Moving
forward, it could be discussed whether writing to log file and writing
to energy trajectory could be coupled (i.e., should log writing frequency
be a multiple of energy writing?).

Fixes #3151

Change-Id: Icb12ecd7c9aedb29138a9a17fb6d130c4f23a06a

diff --git a/src/gromacs/modularsimulator/energyelement.cpp b/src/gromacs/modularsimulator/energyelement.cpp
index cd29cbdafc302e7beda707492e8c2b48175254b5..4ccfe559067bd1f0bcc5971cfee3d0b9fce33425 100644 (file)
--- a/src/gromacs/modularsimulator/energyelement.cpp
+++ b/src/gromacs/modularsimulator/energyelement.cpp
@@ -91,7 +91,6 @@ EnergyElement::EnergyElement(
     energyWritingStep_(-1),
     energyCalculationStep_(-1),
     freeEnergyCalculationStep_(-1),
-    logWritingStep_(-1),
     forceVirialStep_(-1),
     shakeVirialStep_(-1),
     totalVirialStep_(-1),
@@ -207,8 +206,8 @@ ITrajectoryWriterCallbackPtr EnergyElement::registerTrajectoryWriterCallback(Tra
     if (event == TrajectoryEvent::EnergyWritingStep && isMasterRank_)
     {
         return std::make_unique<ITrajectoryWriterCallback>(
-                [this](gmx_mdoutf *mdoutf, Step step, Time time)
-                {write(mdoutf, step, time); });
+                [this](gmx_mdoutf *mdoutf, Step step, Time time, bool writeTrajectory, bool writeLog)
+                {write(mdoutf, step, time, writeTrajectory, writeLog); });
     }
     return nullptr;
 }
@@ -238,12 +237,6 @@ SignallerCallbackPtr EnergyElement::registerEnergyCallback(EnergySignallerEvent
     return nullptr;
 }
 
-SignallerCallbackPtr EnergyElement::registerLoggingCallback()
-{
-    return std::make_unique<SignallerCallback>(
-            [this](Step step, Time){logWritingStep_ = step; });
-}
-
 void EnergyElement::doStep(
         Time time,
         bool isEnergyCalculationStep,
@@ -278,14 +271,8 @@ void EnergyElement::doStep(
             ekind_, muTot_, constr_);
 }
 
-void EnergyElement::write(gmx_mdoutf *outf, Step step, Time time)
+void EnergyElement::write(gmx_mdoutf *outf, Step step, Time time, bool writeTrajectory, bool writeLog)
 {
-    // This gets called by the trajectory writer, which means that
-    // we only write to log when writing energy.
-    // TODO: Think if we need to change that, i.e. writing to log without
-    //       writing to file.
-
-    auto writeLog = logWritingStep_ == step;
     if (writeLog)
     {
         energyOutput_->printHeader(fplog_, step, time);
@@ -297,7 +284,7 @@ void EnergyElement::write(gmx_mdoutf *outf, Step step, Time time)
     // energyOutput_->printAnnealingTemperatures(writeLog ? fplog_ : nullptr, groups_, &(inputrec_->opts));
     Awh *awh = nullptr;
     energyOutput_->printStepToEnergyFile(
-            mdoutf_get_fp_ene(outf), true, do_dr, do_or,
+            mdoutf_get_fp_ene(outf), writeTrajectory, do_dr, do_or,
             writeLog ? fplog_ : nullptr, step, time, fcd_, awh);
 }
 

diff --git a/src/gromacs/modularsimulator/energyelement.h b/src/gromacs/modularsimulator/energyelement.h
index 2d463e0bec6e3140a75930df55454b0c44c4ae65..709157040fcf7cb5f816ab2d950e3ff641e8e826 100644 (file)
--- a/src/gromacs/modularsimulator/energyelement.h
+++ b/src/gromacs/modularsimulator/energyelement.h
@@ -95,7 +95,6 @@ class EnergyElement final :
     public          ITrajectoryWriterClient,
     public          ITrajectorySignallerClient,
     public          IEnergySignallerClient,
-    public          ILoggingSignallerClient,
     public          ICheckpointHelperClient
 {
     public:
@@ -251,8 +250,6 @@ class EnergyElement final :
         registerTrajectoryWriterCallback(TrajectoryEvent event) override;
         //! IEnergySignallerClient implementation
         SignallerCallbackPtr registerEnergyCallback(EnergySignallerEvent event) override;
-        //! ILoggingSignallerClient implementation
-        SignallerCallbackPtr registerLoggingCallback() override;
 
         /*! \brief Save data at energy steps
          *
@@ -271,7 +268,8 @@ class EnergyElement final :
          */
         void write(
             gmx_mdoutf *outf,
-            Step step, Time time);
+            Step step, Time time,
+            bool writeTrajectory, bool writeLog);
 
         //! ICheckpointHelperClient implementation
         void writeCheckpoint(t_state *localState, t_state *globalState) override;
@@ -290,8 +288,6 @@ class EnergyElement final :
         Step       energyCalculationStep_;
         //! The next communicated free energy calculation step
         Step       freeEnergyCalculationStep_;
-        //! The next communicated log writing step
-        Step       logWritingStep_;
 
         //! The force virial tensor
         tensor forceVirial_;

diff --git a/src/gromacs/modularsimulator/modularsimulator.cpp b/src/gromacs/modularsimulator/modularsimulator.cpp
index 7bf1d8accf761f998e4e237ebd4f273d01f77a1b..920b4a92d94ce520c1219490ce5566b75770ec7f 100644 (file)
--- a/src/gromacs/modularsimulator/modularsimulator.cpp
+++ b/src/gromacs/modularsimulator/modularsimulator.cpp
@@ -420,7 +420,6 @@ void ModularSimulator::constructElementsAndSignallers()
     trajectoryElementBuilder.registerWriterClient(energyElementPtr);
     trajectoryElementBuilder.registerSignallerClient(energyElementPtr);
     energySignallerBuilder.registerSignallerClient(energyElementPtr);
-    loggingSignallerBuilder.registerSignallerClient(energyElementPtr);
 
     // Register the simulator itself to the neighbor search / last step signaller
     neighborSearchSignallerBuilder.registerSignallerClient(compat::make_not_null(signalHelper_.get()));
@@ -494,6 +493,7 @@ void ModularSimulator::constructElementsAndSignallers()
     auto trajectoryElement = trajectoryElementBuilder.build(
                 fplog, nfile, fnm, mdrunOptions, cr, outputProvider, mdModulesNotifier,
                 inputrec, top_global, oenv, wcycle, startingBehavior);
+    loggingSignallerBuilder.registerSignallerClient(compat::make_not_null(trajectoryElement.get()));
 
     // Add checkpoint helper here since we need a pointer to the trajectory element and
     // need to register it with the lastStepSignallerBuilder

diff --git a/src/gromacs/modularsimulator/modularsimulatorinterfaces.h b/src/gromacs/modularsimulator/modularsimulatorinterfaces.h
index b29a9675c712d8616e3da1f6a947472ca5e89908..e5fd8698b13b80f552cf0c0d7ac7ff887473c6ff 100644 (file)
--- a/src/gromacs/modularsimulator/modularsimulatorinterfaces.h
+++ b/src/gromacs/modularsimulator/modularsimulatorinterfaces.h
@@ -283,7 +283,7 @@ class ITrajectorySignallerClient
  * Trajectory writing clients are given a pointer to the output file handler,
  * allowing them to write their own trajectory contribution
  */
-typedef std::function<void(gmx_mdoutf*, Step, Time)> ITrajectoryWriterCallback;
+typedef std::function<void(gmx_mdoutf*, Step, Time, bool, bool)> ITrajectoryWriterCallback;
 //! Pointer to the function type for trajectory writing clients
 typedef std::unique_ptr<ITrajectoryWriterCallback> ITrajectoryWriterCallbackPtr;
 

diff --git a/src/gromacs/modularsimulator/statepropagatordata.cpp b/src/gromacs/modularsimulator/statepropagatordata.cpp
index ab173ad9270234b7923320a188cfad926a549014..36b9fc946a1ae67a7c0ecd41d0d5fa964795140b 100644 (file)
--- a/src/gromacs/modularsimulator/statepropagatordata.cpp
+++ b/src/gromacs/modularsimulator/statepropagatordata.cpp
@@ -349,8 +349,13 @@ StatePropagatorData::registerTrajectoryWriterCallback(TrajectoryEvent event)
     if (event == TrajectoryEvent::StateWritingStep)
     {
         return std::make_unique<ITrajectoryWriterCallback>(
-                [this](gmx_mdoutf *outf, Step step, Time time)
-                {write(outf, step, time); });
+                [this](gmx_mdoutf *outf, Step step, Time time, bool writeTrajectory, bool gmx_unused writeLog)
+                {
+                    if (writeTrajectory)
+                    {
+                        write(outf, step, time);
+                    }
+                });
     }
     return nullptr;
 }

diff --git a/src/gromacs/modularsimulator/trajectoryelement.cpp b/src/gromacs/modularsimulator/trajectoryelement.cpp
index be9ce6c2f72afab8c34705a725256fa5ed0cb1c3..1ce632dfc461220fb3cb1fb8a5ef0a719f485274 100644 (file)
--- a/src/gromacs/modularsimulator/trajectoryelement.cpp
+++ b/src/gromacs/modularsimulator/trajectoryelement.cpp
@@ -141,13 +141,16 @@ void TrajectoryElement::elementSetup()
 void TrajectoryElement::scheduleTask(
         Step step, Time time, const RegisterRunFunctionPtr &registerRunFunction)
 {
-    auto writeEnergyThisStep = writeEnergyStep_ == step;
-    auto writeStateThisStep  = writeStateStep_ == step;
-    if (writeEnergyThisStep || writeStateThisStep)
+    const bool writeEnergyThisStep = writeEnergyStep_ == step;
+    const bool writeStateThisStep  = writeStateStep_ == step;
+    const bool writeLogThisStep    = logWritingStep_ == step;
+    if (writeEnergyThisStep || writeStateThisStep || writeLogThisStep)
     {
         (*registerRunFunction)(std::make_unique<SimulatorRunFunction>(
-                                       [this, step, time, writeStateThisStep, writeEnergyThisStep]()
-                                       {write(step, time, writeStateThisStep, writeEnergyThisStep); }));
+                                       [this, step, time, writeStateThisStep,
+                                        writeEnergyThisStep, writeLogThisStep]()
+                                       {write(step, time, writeStateThisStep,
+                                              writeEnergyThisStep, writeLogThisStep); }));
     }
 }
 
@@ -162,20 +165,20 @@ void TrajectoryElement::elementTeardown()
 }
 
 void TrajectoryElement::write(
-        Step step, Time time, bool writeState, bool writeEnergy)
+        Step step, Time time, bool writeState, bool writeEnergy, bool writeLog)
 {
-    if (writeState)
+    if (writeState || writeLog)
     {
         for (auto &callback : runStateCallbacks_)
         {
-            (*callback)(outf_, step, time);
+            (*callback)(outf_, step, time, writeState, writeLog);
         }
     }
-    if (writeEnergy)
+    if (writeEnergy || writeLog)
     {
         for (auto &callback : runEnergyCallbacks_)
         {
-            (*callback)(outf_, step, time);
+            (*callback)(outf_, step, time, writeEnergy, writeLog);
         }
     }
 }
@@ -187,6 +190,12 @@ SignallerCallbackPtr TrajectoryElement::registerLastStepCallback()
             [this](Step step, Time gmx_unused time){this->lastStep_ = step; });
 }
 
+SignallerCallbackPtr TrajectoryElement::registerLoggingCallback()
+{
+    return std::make_unique<SignallerCallback>(
+            [this](Step step, Time){logWritingStep_ = step; });
+}
+
 void TrajectoryElementBuilder::registerSignallerClient(
         compat::not_null<ITrajectorySignallerClient*> client)
 {

diff --git a/src/gromacs/modularsimulator/trajectoryelement.h b/src/gromacs/modularsimulator/trajectoryelement.h
index 5bb4d82ffc096f515926707de728d0ec3f0fe8b0..d845a0fa959467575347a03c240b852ecf7aa7c6 100644 (file)
--- a/src/gromacs/modularsimulator/trajectoryelement.h
+++ b/src/gromacs/modularsimulator/trajectoryelement.h
@@ -82,7 +82,8 @@ enum class StartingBehavior;
 class TrajectoryElement final :
     public              ISimulatorElement,
     public              ISignaller,
-    public              ILastStepSignallerClient
+    public              ILastStepSignallerClient,
+    public              ILoggingSignallerClient
 {
     public:
         friend class TrajectoryElementBuilder;
@@ -165,10 +166,15 @@ class TrajectoryElement final :
         Step writeEnergyStep_;
         //! The next state writing step
         Step writeStateStep_;
+        //! The next communicated log writing step
+        Step logWritingStep_;
 
         //! The output object
         gmx_mdoutf *outf_;
 
+        //! ILoggingSignallerClient implementation
+        SignallerCallbackPtr registerLoggingCallback() override;
+
         /*
          * Signaller
          */
@@ -212,7 +218,7 @@ class TrajectoryElement final :
         //! }
 
         //! The writing function - calls the clients to get their contributions
-        void write(Step step, Time time, bool writeState, bool writeEnergy);
+        void write(Step step, Time time, bool writeState, bool writeEnergy, bool writeLog);
 };
 
 /*! \libinternal