Follow up to "Separate StatePropagatorData element" (!363)

StatePropagatorData, EnergyData, and FreeEnergyPerturbationData
have all been separated into a pure data class and an Element
member object. The three were intended to be following the same
design principles, but were merged in three separate MR. The
design was improved in reviews of the EnergyData and
FreeEnergyPerturbationData MRs, leaving StatePropagatorData
with some of the design flaws ironed out in the other two.
This commit adapts StatepropagatorData and its member Element
to the improved design. Specifically:

* Build the Element in the constructor of StatepropagatorData.
  To this end, the change in signature of the constructor is
  partially rolled back to have all required data available.
* The element is accessed using a simple getter method, doing
  away with building at first invocation or throwing exceptions
  when accessed more than once.

diff --git a/src/gromacs/modularsimulator/modularsimulator.cpp b/src/gromacs/modularsimulator/modularsimulator.cpp
index 594fc55941a99b61c25c3bf0df1a201c772b3eed..acce02ac02ff131834cb5ad9aff242b97b1683aa 100644 (file)
--- a/src/gromacs/modularsimulator/modularsimulator.cpp
+++ b/src/gromacs/modularsimulator/modularsimulator.cpp
@@ -172,10 +172,7 @@ std::unique_ptr<ISimulatorElement> ModularSimulatorAlgorithmBuilder::buildIntegr
                 inputrec->delta_t, statePropagatorDataPtr, mdAtoms, wcycle);
 
         addToCallListAndMove(std::move(forceElement), elementCallList, elementsOwnershipList);
-        auto stateElement = compat::make_not_null(statePropagatorDataPtr->element(
-                fplog, cr, inputrec->nstxout, inputrec->nstvout, inputrec->nstfout,
-                inputrec->nstxout_compressed, freeEnergyPerturbationDataPtr, fr->bMolPBC,
-                mdrunOptions.writeConfout, opt2fn("-c", nfile, fnm), inputrec, top_global));
+        auto stateElement = compat::make_not_null(statePropagatorDataPtr->element());
         trajectoryElementBuilder->registerWriterClient(stateElement);
         trajectorySignallerBuilder->registerSignallerClient(stateElement);
         lastStepSignallerBuilder->registerSignallerClient(stateElement);
@@ -288,10 +285,7 @@ std::unique_ptr<ISimulatorElement> ModularSimulatorAlgorithmBuilder::buildIntegr
             addToCallListAndMove(std::move(constraintElement), elementCallList, elementsOwnershipList);
         }
         addToCallList(compat::make_not_null(computeGlobalsElement.get()), elementCallList);
-        auto stateElement = compat::make_not_null(statePropagatorDataPtr->element(
-                fplog, cr, inputrec->nstxout, inputrec->nstvout, inputrec->nstfout,
-                inputrec->nstxout_compressed, freeEnergyPerturbationDataPtr, fr->bMolPBC,
-                mdrunOptions.writeConfout, opt2fn("-c", nfile, fnm), inputrec, top_global));
+        auto stateElement = compat::make_not_null(statePropagatorDataPtr->element());
         trajectoryElementBuilder->registerWriterClient(stateElement);
         trajectorySignallerBuilder->registerSignallerClient(stateElement);
         lastStepSignallerBuilder->registerSignallerClient(stateElement);

diff --git a/src/gromacs/modularsimulator/simulatoralgorithm.cpp b/src/gromacs/modularsimulator/simulatoralgorithm.cpp
index e2f3930efbcfa4ce0e37c620fb25dbc5bc0b3d75..87e776e9cf4eb27553e6bbdf6947caa98ba78eeb 100644 (file)
--- a/src/gromacs/modularsimulator/simulatoralgorithm.cpp
+++ b/src/gromacs/modularsimulator/simulatoralgorithm.cpp
@@ -427,7 +427,9 @@ ModularSimulatorAlgorithm ModularSimulatorAlgorithmBuilder::constructElementsAnd
             algorithm.freeEnergyPerturbationData_.get();
 
     algorithm.statePropagatorData_ = std::make_unique<StatePropagatorData>(
-            top_global->natoms, cr, state_global, fr->nbv->useGpu(), inputrec, mdAtoms->mdatoms());
+            top_global->natoms, fplog, cr, state_global, fr->nbv->useGpu(),
+            freeEnergyPerturbationDataPtr, fr->bMolPBC, mdrunOptions.writeConfout,
+            opt2fn("-c", nfile, fnm), inputrec, mdAtoms->mdatoms(), top_global);
     auto statePropagatorDataPtr = compat::make_not_null(algorithm.statePropagatorData_.get());
 
     algorithm.energyData_ = std::make_unique<EnergyData>(

diff --git a/src/gromacs/modularsimulator/statepropagatordata.cpp b/src/gromacs/modularsimulator/statepropagatordata.cpp
index ce6ea478cacc4b801b18d7b9abf06badf42adbde..2e43a870b5b4291ff0d3f08df30f1661c0656709 100644 (file)
--- a/src/gromacs/modularsimulator/statepropagatordata.cpp
+++ b/src/gromacs/modularsimulator/statepropagatordata.cpp
@@ -63,25 +63,41 @@
 
 namespace gmx
 {
-StatePropagatorData::StatePropagatorData(int               numAtoms,
-                                         const t_commrec*  cr,
-                                         t_state*          globalState,
-                                         bool              useGPU,
-                                         const t_inputrec* inputrec,
-                                         const t_mdatoms*  mdatoms) :
+StatePropagatorData::StatePropagatorData(int                         numAtoms,
+                                         FILE*                       fplog,
+                                         const t_commrec*            cr,
+                                         t_state*                    globalState,
+                                         bool                        useGPU,
+                                         FreeEnergyPerturbationData* freeEnergyPerturbationData,
+                                         bool               canMoleculesBeDistributedOverPBC,
+                                         bool               writeFinalConfiguration,
+                                         const std::string& finalConfigurationFilename,
+                                         const t_inputrec*  inputrec,
+                                         const t_mdatoms*   mdatoms,
+                                         const gmx_mtop_t*  globalTop) :
     totalNumAtoms_(numAtoms),
     localNAtoms_(0),
+    box_{ { 0 } },
+    previousBox_{ { 0 } },
     ddpCount_(0),
+    element_(std::make_unique<Element>(this,
+                                       fplog,
+                                       cr,
+                                       inputrec->nstxout,
+                                       inputrec->nstvout,
+                                       inputrec->nstfout,
+                                       inputrec->nstxout_compressed,
+                                       freeEnergyPerturbationData,
+                                       canMoleculesBeDistributedOverPBC,
+                                       writeFinalConfiguration,
+                                       finalConfigurationFilename,
+                                       inputrec,
+                                       globalTop)),
     vvResetVelocities_(false),
     isRegularSimulationEnd_(false),
     lastStep_(-1),
     globalState_(globalState)
 {
-    // Initialize these here, as box_{{0}} in the initialization list
-    // is confusing uncrustify and doxygen
-    clear_mat(box_);
-    clear_mat(previousBox_);
-
     bool stateHasVelocities;
     // Local state only becomes valid now.
     if (DOMAINDECOMP(cr))
@@ -140,31 +156,8 @@ StatePropagatorData::StatePropagatorData(int               numAtoms,
     }
 }
 
-StatePropagatorData::Element* StatePropagatorData::element(FILE*            fplog,
-                                                           const t_commrec* cr,
-                                                           int              nstxout,
-                                                           int              nstvout,
-                                                           int              nstfout,
-                                                           int              nstxout_compressed,
-                                                           FreeEnergyPerturbationData* freeEnergyPerturbationData,
-                                                           bool canMoleculesBeDistributedOverPBC,
-                                                           bool writeFinalConfiguration,
-                                                           std::string finalConfigurationFilename,
-                                                           const t_inputrec* inputrec,
-                                                           const gmx_mtop_t* globalTop)
-{
-    if (!element_)
-    {
-        element_ = std::make_unique<Element>(
-                this, fplog, cr, nstxout, nstvout, nstfout, nstxout_compressed,
-                freeEnergyPerturbationData, canMoleculesBeDistributedOverPBC,
-                writeFinalConfiguration, std::move(finalConfigurationFilename), inputrec, globalTop);
-    }
-    else
-    {
-        GMX_THROW(InconsistentInputError(
-                "Attempted to build StatePropagatorData::Element more than once."));
-    }
+StatePropagatorData::Element* StatePropagatorData::element()
+{
     return element_.get();
 }
 

diff --git a/src/gromacs/modularsimulator/statepropagatordata.h b/src/gromacs/modularsimulator/statepropagatordata.h
index 16b6c97d311d306cfc507275fc0e54efe8a44b44..0487be3a5071c0ceeb9051386fb02a35e634d1b9 100644 (file)
--- a/src/gromacs/modularsimulator/statepropagatordata.h
+++ b/src/gromacs/modularsimulator/statepropagatordata.h
@@ -91,12 +91,18 @@ class StatePropagatorData final
 {
 public:
     //! Constructor
-    StatePropagatorData(int               numAtoms,
-                        const t_commrec*  cr,
-                        t_state*          globalState,
-                        bool              useGPU,
-                        const t_inputrec* inputrec,
-                        const t_mdatoms*  mdatoms);
+    StatePropagatorData(int                         numAtoms,
+                        FILE*                       fplog,
+                        const t_commrec*            cr,
+                        t_state*                    globalState,
+                        bool                        useGPU,
+                        FreeEnergyPerturbationData* freeEnergyPerturbationData,
+                        bool                        canMoleculesBeDistributedOverPBC,
+                        bool                        writeFinalConfiguration,
+                        const std::string&          finalConfigurationFilename,
+                        const t_inputrec*           inputrec,
+                        const t_mdatoms*            mdatoms,
+                        const gmx_mtop_t*           globalTop);
 
     // Allow access to state
     //! Get write access to position vector
@@ -131,18 +137,7 @@ public:
     //! The element taking part in the simulator loop
     class Element;
     //! Get pointer to element (whose lifetime is managed by this)
-    Element* element(FILE*                       fplog,
-                     const t_commrec*            cr,
-                     int                         nstxout,
-                     int                         nstvout,
-                     int                         nstfout,
-                     int                         nstxout_compressed,
-                     FreeEnergyPerturbationData* freeEnergyPerturbationData,
-                     bool                        canMoleculesBeDistributedOverPBC,
-                     bool                        writeFinalConfiguration,
-                     std::string                 finalConfigurationFilename,
-                     const t_inputrec*           inputrec,
-                     const gmx_mtop_t*           globalTop);
+    Element* element();
     //! Initial set up for the associated element
     void setup();