From f1c0599ca0a471930ad680dafdcf846104c0b63e Mon Sep 17 00:00:00 2001 From: Mark Abraham Date: Tue, 30 Mar 2021 09:44:29 +0000 Subject: [PATCH] Move responsibility for checking bondeds in reverse topology The reverse topology is a data structure distributed across all domains that should refer exactly once to all bonded interactions present in the global topology. We check that this is true at run time, but need a complex implementation to avoid doing extra collective communication that supports running what is effectively an assertion that the reverse topology construction is correct. Past attempts to improve this (e.g. 8b6c99a1) left the code in an intermediate state where the responsibility for this check was mostly implemented in do_md rather than compute_globals. That gave the impression of compute_globals() returning an integer parameter. However as above it's really an implementation detail of a check of the reverse topology, and so the logic and memory for that check should be in the domain decomposition module and object. This change move the details of that check to the rest of the code that handles the reverse topology. It renames several variables and adds documentation for them. This simplifies parts of the implementation of the modular simulator, and prepares for implementing the proposal for #3887 by making all outputs from global reduction local to the module that consumes them. Refs #3421 Refs #3887 --- src/gromacs/domdec/domdec.h | 29 ++ src/gromacs/domdec/domdec_struct.h | 2 - src/gromacs/domdec/domdec_topology.cpp | 374 +++++++++++------- src/gromacs/domdec/partition.cpp | 21 - src/gromacs/domdec/partition.h | 20 - src/gromacs/mdlib/md_support.cpp | 2 - src/gromacs/mdlib/md_support.h | 1 - src/gromacs/mdlib/stat.cpp | 11 +- src/gromacs/mdlib/stat.h | 1 - src/gromacs/mdlib/update_vv.cpp | 111 +++--- src/gromacs/mdlib/update_vv.h | 79 ++-- src/gromacs/mdrun/md.cpp | 76 ++-- src/gromacs/mdrun/mimic.cpp | 56 +-- src/gromacs/mdrun/minimize.cpp | 1 - src/gromacs/mdrun/rerun.cpp | 56 +-- .../computeglobalselement.cpp | 86 ++-- src/gromacs/modularsimulator/domdechelper.cpp | 39 +- src/gromacs/modularsimulator/domdechelper.h | 42 +- .../modularsimulator/simulatoralgorithm.cpp | 16 - .../modularsimulator/simulatoralgorithm.h | 7 - 20 files changed, 499 insertions(+), 531 deletions(-) diff --git a/src/gromacs/domdec/domdec.h b/src/gromacs/domdec/domdec.h index ef4288a534..44094f0dc5 100644 --- a/src/gromacs/domdec/domdec.h +++ b/src/gromacs/domdec/domdec.h @@ -281,6 +281,35 @@ void dd_make_reverse_top(FILE* fplog, const t_inputrec& inputrec, DDBondedChecking ddBondedChecking); +/*! \brief Return whether the total bonded interaction count across + * domains should be checked this step. */ +bool shouldCheckNumberOfBondedInteractions(const gmx_domdec_t& dd); + +//! Return the number of bonded interactions in this domain. +int numBondedInteractions(const gmx_domdec_t& dd); + +/*! \brief Set total bonded interaction count across domains. */ +void setNumberOfBondedInteractionsOverAllDomains(const gmx_domdec_t& dd, int newValue); + +/*! \brief Check whether bonded interactions are missing from the reverse topology + * produced by domain decomposition. + * + * Must only be called when DD is active. + * + * \param[in] mdlog Logger + * \param[in] cr Communication object + * \param[in] top_global Global topology for the error message + * \param[in] top_local Local topology for the error message + * \param[in] x Position vector for the error message + * \param[in] box Box matrix for the error message + */ +void checkNumberOfBondedInteractions(const gmx::MDLogger& mdlog, + t_commrec* cr, + const gmx_mtop_t& top_global, + const gmx_localtop_t* top_local, + gmx::ArrayRef x, + const matrix box); + /*! \brief Generate the local topology and virtual site data */ void dd_make_local_top(struct gmx_domdec_t* dd, struct gmx_domdec_zones_t* zones, diff --git a/src/gromacs/domdec/domdec_struct.h b/src/gromacs/domdec/domdec_struct.h index 8612abf5d7..e4e2c784ce 100644 --- a/src/gromacs/domdec/domdec_struct.h +++ b/src/gromacs/domdec/domdec_struct.h @@ -200,8 +200,6 @@ struct gmx_domdec_t /* Global atom number to interaction list */ std::unique_ptr reverse_top; - int nbonded_global = 0; - int nbonded_local = 0; /* Whether we have non-self exclusion */ bool haveExclusions = false; diff --git a/src/gromacs/domdec/domdec_topology.cpp b/src/gromacs/domdec/domdec_topology.cpp index c4a5ac2cd8..2380a813e8 100644 --- a/src/gromacs/domdec/domdec_topology.cpp +++ b/src/gromacs/domdec/domdec_topology.cpp @@ -52,6 +52,7 @@ #include #include +#include #include #include "gromacs/domdec/domdec.h" @@ -123,9 +124,9 @@ struct thread_work_t InteractionDefinitions idef; /**< Partial local topology */ std::unique_ptr vsitePbc = nullptr; /**< vsite PBC structure */ - int nbonded = 0; /**< The number of bondeds in this struct */ - ListOfLists excl; /**< List of exclusions */ - int excl_count = 0; /**< The total exclusion count for \p excl */ + int numBondedInteractions = 0; /**< The number of bonded interactions observed */ + ListOfLists excl; /**< List of exclusions */ + int excl_count = 0; /**< The total exclusion count for \p excl */ }; /*! \brief Options for setting up gmx_reverse_top_t */ @@ -174,8 +175,34 @@ struct gmx_reverse_top_t::Impl //! \brief Intermolecular reverse ilist reverse_ilist_t ril_intermol; - //! The interaction count for the interactions that have to be present - int numInteractionsToCheck; + /*! \brief Data to help check reverse topology construction + * + * Partitioning could incorrectly miss a bonded interaction. + * However, checking for that requires a global communication + * stage, which does not otherwise happen during partitioning. So, + * for performance, we do that alongside the first global energy + * reduction after a new DD is made. These variables handle + * whether the check happens, its input for this domain, output + * across all domains, and the expected value it should match. */ + /*! \{ */ + /*! \brief Number of bonded interactions found in the reverse + * topology for this domain. */ + int numBondedInteractions = 0; + /*! \brief Whether to check at the next global communication + * stage the total number of bonded interactions found. + * + * Cleared after that number is found. */ + bool shouldCheckNumberOfBondedInteractions = false; + /*! \brief The total number of bonded interactions found in + * the reverse topology across all domains. + * + * Only has a value after reduction across all ranks, which is + * removed when it is again time to check after a new + * partition. */ + std::optional numBondedInteractionsOverAllDomains; + //! The number of bonded interactions computed from the full topology + int expectedNumGlobalBondedInteractions = 0; + /*! \} */ /* Work data structures for multi-threading */ //! \brief Thread work array for local topology generation @@ -386,11 +413,11 @@ static void printMissingInteractionsAtoms(const gmx::MDLogger& mdlog, } } -void dd_print_missing_interactions(const gmx::MDLogger& mdlog, - t_commrec* cr, - int local_count, - const gmx_mtop_t& top_global, - const gmx_localtop_t* top_local, +void dd_print_missing_interactions(const gmx::MDLogger& mdlog, + t_commrec* cr, + int numBondedInteractionsOverAllDomains, + const gmx_mtop_t& top_global, + const gmx_localtop_t* top_local, gmx::ArrayRef x, const matrix box) { @@ -402,7 +429,8 @@ void dd_print_missing_interactions(const gmx::MDLogger& mdlog, "Not all bonded interactions have been properly assigned to the domain " "decomposition cells"); - const int ndiff_tot = local_count - dd->nbonded_global; + const int ndiff_tot = numBondedInteractionsOverAllDomains + - dd->reverse_top->impl_->expectedNumGlobalBondedInteractions; for (int ftype = 0; ftype < F_NRE; ftype++) { @@ -415,8 +443,8 @@ void dd_print_missing_interactions(const gmx::MDLogger& mdlog, if (DDMASTER(dd)) { GMX_LOG(mdlog.warning).appendText("A list of missing interactions:"); - int rest_global = dd->nbonded_global; - int rest_local = local_count; + int rest_global = dd->reverse_top->impl_->expectedNumGlobalBondedInteractions; + int rest = numBondedInteractionsOverAllDomains; for (int ftype = 0; ftype < F_NRE; ftype++) { /* In the reverse and local top all constraints are merged @@ -440,11 +468,11 @@ void dd_print_missing_interactions(const gmx::MDLogger& mdlog, -ndiff); } rest_global -= n; - rest_local -= cl[ftype]; + rest -= cl[ftype]; } } - int ndiff = rest_local - rest_global; + int ndiff = rest - rest_global; if (ndiff != 0) { GMX_LOG(mdlog.warning).appendTextFormatted("%20s of %6d missing %6d", "exclusions", rest_global, -ndiff); @@ -470,7 +498,7 @@ void dd_print_missing_interactions(const gmx::MDLogger& mdlog, "two-body cut-off distance (%g nm), see option -rdd, for pairs and tabulated bonds " "also see option -ddcheck", -ndiff_tot, - cr->dd->nbonded_global, + dd->reverse_top->impl_->expectedNumGlobalBondedInteractions, dd_cutoff_multibody(dd), dd_cutoff_twobody(dd)); } @@ -693,10 +721,10 @@ gmx_reverse_top_t::Impl::Impl(const gmx_mtop_t& mtop, fprintf(debug, "The total size of the atom to interaction index is %d integers\n", ril_mt_tot_size); } - numInteractionsToCheck = 0; + expectedNumGlobalBondedInteractions = 0; for (const gmx_molblock_t& molblock : mtop.molblock) { - numInteractionsToCheck += molblock.nmol * nint_mt[molblock.type]; + expectedNumGlobalBondedInteractions += molblock.nmol * nint_mt[molblock.type]; } /* Make an intermolecular reverse top, if necessary */ @@ -711,7 +739,7 @@ gmx_reverse_top_t::Impl::Impl(const gmx_mtop_t& mtop, GMX_RELEASE_ASSERT(mtop.intermolecular_ilist, "We should have an ilist when intermolecular interactions are on"); - numInteractionsToCheck += make_reverse_ilist( + expectedNumGlobalBondedInteractions += make_reverse_ilist( *mtop.intermolecular_ilist, &atoms_global, options, AtomLinkRule::FirstAtom, &ril_intermol); } @@ -769,8 +797,6 @@ void dd_make_reverse_top(FILE* fplog, dd->reverse_top = std::make_unique( mtop, inputrec.efep != FreeEnergyPerturbationType::No, rtOptions); - dd->nbonded_global = dd->reverse_top->impl_->numInteractionsToCheck; - dd->haveExclusions = false; for (const gmx_molblock_t& molb : mtop.molblock) { @@ -1068,35 +1094,40 @@ static void combine_idef(InteractionDefinitions* dest, gmx::ArrayRef index, - gmx::ArrayRef rtil, - gmx_bool bInterMolInteractions, - int ind_start, - int ind_end, - const gmx_ga2la_t& ga2la, - const gmx_domdec_zones_t* zones, - const gmx_molblock_t* molb, - gmx_bool bRCheckMB, - const ivec rcheck, - gmx_bool bRCheck2B, - real rc2, - t_pbc* pbc_null, - rvec* cg_cm, - const t_iparams* ip_in, - InteractionDefinitions* idef, - int iz, - const DDBondedChecking ddBondedChecking, - int* nbonded_local) +static inline int assign_interactions_atom(int i, + int i_gl, + int mol, + int i_mol, + int numAtomsInMolecule, + gmx::ArrayRef index, + gmx::ArrayRef rtil, + gmx_bool bInterMolInteractions, + int ind_start, + int ind_end, + const gmx_ga2la_t& ga2la, + const gmx_domdec_zones_t* zones, + const gmx_molblock_t* molb, + gmx_bool bRCheckMB, + const ivec rcheck, + gmx_bool bRCheck2B, + real rc2, + t_pbc* pbc_null, + rvec* cg_cm, + const t_iparams* ip_in, + InteractionDefinitions* idef, + int iz, + const DDBondedChecking ddBondedChecking) { gmx::ArrayRef iZones = zones->iZones; + int numBondedInteractions = 0; + int j = ind_start; while (j < ind_end) { @@ -1185,7 +1216,7 @@ static inline void check_assign_interactions_atom(int i, else { /* Assign this multi-body bonded interaction to - * the local node if we have all the atoms involved + * the local domain if we have all the atoms involved * (local or communicated) and the minimum zone shift * in each dimension is zero, for dimensions * with 2 DD cells an extra check may be necessary. @@ -1253,12 +1284,14 @@ static inline void check_assign_interactions_atom(int i, if (ddBondedChecking == DDBondedChecking::All || !(interaction_function[ftype].flags & IF_LIMZERO)) { - (*nbonded_local)++; + numBondedInteractions++; } } } j += 1 + nral_rt(ftype); } + + return numBondedInteractions; } /*! \brief This function looks up and assigns bonded interactions for zone iz. @@ -1289,7 +1322,7 @@ static int make_bondeds_zone(gmx_reverse_top_t* rt, const auto ddBondedChecking = rt->impl_->options.ddBondedChecking; - int nbonded_local = 0; + int numBondedInteractions = 0; for (int i : atomRange) { @@ -1300,30 +1333,29 @@ static int make_bondeds_zone(gmx_reverse_top_t* rt, gmx::ArrayRef index = rt->impl_->ril_mt[mt].index; gmx::ArrayRef rtil = rt->impl_->ril_mt[mt].il; - check_assign_interactions_atom(i, - i_gl, - mol, - i_mol, - rt->impl_->ril_mt[mt].numAtomsInMolecule, - index, - rtil, - FALSE, - index[i_mol], - index[i_mol + 1], - ga2la, - zones, - &molb[mb], - bRCheckMB, - rcheck, - bRCheck2B, - rc2, - pbc_null, - cg_cm, - ip_in, - idef, - izone, - ddBondedChecking, - &nbonded_local); + numBondedInteractions += assign_interactions_atom(i, + i_gl, + mol, + i_mol, + rt->impl_->ril_mt[mt].numAtomsInMolecule, + index, + rtil, + FALSE, + index[i_mol], + index[i_mol + 1], + ga2la, + zones, + &molb[mb], + bRCheckMB, + rcheck, + bRCheck2B, + rc2, + pbc_null, + cg_cm, + ip_in, + idef, + izone, + ddBondedChecking); if (rt->impl_->bIntermolecularInteractions) @@ -1332,34 +1364,33 @@ static int make_bondeds_zone(gmx_reverse_top_t* rt, index = rt->impl_->ril_intermol.index; rtil = rt->impl_->ril_intermol.il; - check_assign_interactions_atom(i, - i_gl, - mol, - i_mol, - rt->impl_->ril_mt[mt].numAtomsInMolecule, - index, - rtil, - TRUE, - index[i_gl], - index[i_gl + 1], - ga2la, - zones, - &molb[mb], - bRCheckMB, - rcheck, - bRCheck2B, - rc2, - pbc_null, - cg_cm, - ip_in, - idef, - izone, - ddBondedChecking, - &nbonded_local); + numBondedInteractions += assign_interactions_atom(i, + i_gl, + mol, + i_mol, + rt->impl_->ril_mt[mt].numAtomsInMolecule, + index, + rtil, + TRUE, + index[i_gl], + index[i_gl + 1], + ga2la, + zones, + &molb[mb], + bRCheckMB, + rcheck, + bRCheck2B, + rc2, + pbc_null, + cg_cm, + ip_in, + idef, + izone, + ddBondedChecking); } } - return nbonded_local; + return numBondedInteractions; } /*! \brief Set the exclusion data for i-zone \p iz */ @@ -1438,19 +1469,19 @@ static void make_exclusions_zone(ArrayRef globalAtomInd } /*! \brief Generate and store all required local bonded interactions in \p idef and local exclusions in \p lexcls */ -static int make_local_bondeds_excls(gmx_domdec_t* dd, - gmx_domdec_zones_t* zones, - const gmx_mtop_t& mtop, - const int* cginfo, - gmx_bool bRCheckMB, - ivec rcheck, - gmx_bool bRCheck2B, - real rc, - t_pbc* pbc_null, - rvec* cg_cm, - InteractionDefinitions* idef, - ListOfLists* lexcls, - int* excl_count) +static void make_local_bondeds_excls(gmx_domdec_t* dd, + gmx_domdec_zones_t* zones, + const gmx_mtop_t& mtop, + const int* cginfo, + gmx_bool bRCheckMB, + ivec rcheck, + gmx_bool bRCheck2B, + real rc, + t_pbc* pbc_null, + rvec* cg_cm, + InteractionDefinitions* idef, + ListOfLists* lexcls, + int* excl_count) { int nzone_bondeds = 0; @@ -1475,7 +1506,7 @@ static int make_local_bondeds_excls(gmx_domdec_t* dd, /* Clear the counts */ idef->clear(); - int nbonded_local = 0; + dd->reverse_top->impl_->numBondedInteractions = 0; lexcls->clear(); *excl_count = 0; @@ -1506,21 +1537,22 @@ static int make_local_bondeds_excls(gmx_domdec_t* dd, idef_t->clear(); } - rt->impl_->th_work[thread].nbonded = make_bondeds_zone(rt, - dd->globalAtomIndices, - *dd->ga2la, - zones, - mtop.molblock, - bRCheckMB, - rcheck, - bRCheck2B, - rc2, - pbc_null, - cg_cm, - idef->iparams.data(), - idef_t, - izone, - gmx::Range(cg0t, cg1t)); + rt->impl_->th_work[thread].numBondedInteractions = + make_bondeds_zone(rt, + dd->globalAtomIndices, + *dd->ga2la, + zones, + mtop.molblock, + bRCheckMB, + rcheck, + bRCheck2B, + rc2, + pbc_null, + cg_cm, + idef->iparams.data(), + idef_t, + izone, + gmx::Range(cg0t, cg1t)); if (izone < numIZonesForExclusions) { @@ -1561,7 +1593,7 @@ static int make_local_bondeds_excls(gmx_domdec_t* dd, for (const thread_work_t& th_work : rt->impl_->th_work) { - nbonded_local += th_work.nbonded; + dd->reverse_top->impl_->numBondedInteractions += th_work.numBondedInteractions; } if (izone < numIZonesForExclusions) @@ -1577,12 +1609,70 @@ static int make_local_bondeds_excls(gmx_domdec_t* dd, } } + // Note that it's possible for this to still be true from the last + // time it was set, e.g. if repartitioning was triggered before + // global communication that would have acted on the true + // value. This could happen for example when replica exchange took + // place soon after a partition. + dd->reverse_top->impl_->shouldCheckNumberOfBondedInteractions = true; + // Clear the old global value, which is now invalid + dd->reverse_top->impl_->numBondedInteractionsOverAllDomains.reset(); + if (debug) { fprintf(debug, "We have %d exclusions, check count %d\n", lexcls->numElements(), *excl_count); } +} + +bool shouldCheckNumberOfBondedInteractions(const gmx_domdec_t& dd) +{ + return dd.reverse_top->impl_->shouldCheckNumberOfBondedInteractions; +} - return nbonded_local; +int numBondedInteractions(const gmx_domdec_t& dd) +{ + return dd.reverse_top->impl_->numBondedInteractions; +} + +void setNumberOfBondedInteractionsOverAllDomains(const gmx_domdec_t& dd, int newValue) +{ + GMX_RELEASE_ASSERT(!dd.reverse_top->impl_->numBondedInteractionsOverAllDomains.has_value(), + "Cannot set number of bonded interactions because it is already set"); + dd.reverse_top->impl_->numBondedInteractionsOverAllDomains.emplace(newValue); +} + +void checkNumberOfBondedInteractions(const gmx::MDLogger& mdlog, + t_commrec* cr, + const gmx_mtop_t& top_global, + const gmx_localtop_t* top_local, + gmx::ArrayRef x, + const matrix box) +{ + GMX_RELEASE_ASSERT( + DOMAINDECOMP(cr), + "No need to check number of bonded interactions when not using domain decomposition"); + if (cr->dd->reverse_top->impl_->shouldCheckNumberOfBondedInteractions) + { + GMX_RELEASE_ASSERT(cr->dd->reverse_top->impl_->numBondedInteractionsOverAllDomains.has_value(), + "The check for the total number of bonded interactions requires the " + "value to have been reduced across all domains"); + if (cr->dd->reverse_top->impl_->numBondedInteractionsOverAllDomains.value() + != cr->dd->reverse_top->impl_->expectedNumGlobalBondedInteractions) + { + dd_print_missing_interactions( + mdlog, + cr, + cr->dd->reverse_top->impl_->numBondedInteractionsOverAllDomains.value(), + top_global, + top_local, + x, + box); // Does not return + } + // Now that the value is set and the check complete, future + // global communication should not compute the value until + // after the next partitioning. + cr->dd->reverse_top->impl_->shouldCheckNumberOfBondedInteractions = false; + } } void dd_make_local_top(gmx_domdec_t* dd, @@ -1663,19 +1753,19 @@ void dd_make_local_top(gmx_domdec_t* dd, } } - dd->nbonded_local = make_local_bondeds_excls(dd, - zones, - mtop, - fr->cginfo.data(), - bRCheckMB, - rcheck, - bRCheck2B, - rc, - pbc_null, - cgcm_or_x, - <op->idef, - <op->excls, - &nexcl); + make_local_bondeds_excls(dd, + zones, + mtop, + fr->cginfo.data(), + bRCheckMB, + rcheck, + bRCheck2B, + rc, + pbc_null, + cgcm_or_x, + <op->idef, + <op->excls, + &nexcl); /* The ilist is not sorted yet, * we can only do this when we have the charge arrays. diff --git a/src/gromacs/domdec/partition.cpp b/src/gromacs/domdec/partition.cpp index 23bd260326..a0cffa2abe 100644 --- a/src/gromacs/domdec/partition.cpp +++ b/src/gromacs/domdec/partition.cpp @@ -3364,24 +3364,3 @@ void dd_partition_system(FILE* fplog, wallcycle_stop(wcycle, ewcDOMDEC); } - -/*! \brief Check whether bonded interactions are missing, if appropriate */ -void checkNumberOfBondedInteractions(const gmx::MDLogger& mdlog, - t_commrec* cr, - int totalNumberOfBondedInteractions, - const gmx_mtop_t& top_global, - const gmx_localtop_t* top_local, - gmx::ArrayRef x, - const matrix box, - bool* shouldCheckNumberOfBondedInteractions) -{ - if (*shouldCheckNumberOfBondedInteractions) - { - if (totalNumberOfBondedInteractions != cr->dd->nbonded_global) - { - dd_print_missing_interactions( - mdlog, cr, totalNumberOfBondedInteractions, top_global, top_local, x, box); // Does not return - } - *shouldCheckNumberOfBondedInteractions = false; - } -} diff --git a/src/gromacs/domdec/partition.h b/src/gromacs/domdec/partition.h index da3c066737..705d666e26 100644 --- a/src/gromacs/domdec/partition.h +++ b/src/gromacs/domdec/partition.h @@ -132,24 +132,4 @@ void dd_partition_system(FILE* fplog, gmx_wallcycle* wcycle, gmx_bool bVerbose); -/*! \brief Check whether bonded interactions are missing, if appropriate - * - * \param[in] mdlog Logger - * \param[in] cr Communication object - * \param[in] totalNumberOfBondedInteractions Result of the global reduction over the number of bonds treated in each domain - * \param[in] top_global Global topology for the error message - * \param[in] top_local Local topology for the error message - * \param[in] x Position vector for the error message - * \param[in] box Box matrix for the error message - * \param[in,out] shouldCheckNumberOfBondedInteractions Whether we should do the check. Always set to false. - */ -void checkNumberOfBondedInteractions(const gmx::MDLogger& mdlog, - t_commrec* cr, - int totalNumberOfBondedInteractions, - const gmx_mtop_t& top_global, - const gmx_localtop_t* top_local, - gmx::ArrayRef x, - const matrix box, - bool* shouldCheckNumberOfBondedInteractions); - #endif diff --git a/src/gromacs/mdlib/md_support.cpp b/src/gromacs/mdlib/md_support.cpp index 0d7b2c496a..dd12bbe7d2 100644 --- a/src/gromacs/mdlib/md_support.cpp +++ b/src/gromacs/mdlib/md_support.cpp @@ -301,7 +301,6 @@ void compute_globals(gmx_global_stat* gstat, gmx::ArrayRef constraintsRmsdData, gmx::SimulationSignaller* signalCoordinator, const matrix lastbox, - int* totalNumberOfBondedInteractions, gmx_bool* bSumEkinhOld, const int flags) { @@ -372,7 +371,6 @@ void compute_globals(gmx_global_stat* gstat, bStopCM ? vcm : nullptr, signalBuffer.size(), signalBuffer.data(), - totalNumberOfBondedInteractions, *bSumEkinhOld, flags); wallcycle_stop(wcycle, ewcMoveE); diff --git a/src/gromacs/mdlib/md_support.h b/src/gromacs/mdlib/md_support.h index aad947007e..1975239425 100644 --- a/src/gromacs/mdlib/md_support.h +++ b/src/gromacs/mdlib/md_support.h @@ -132,7 +132,6 @@ void compute_globals(gmx_global_stat* gstat, gmx::ArrayRef constraintsRmsdData, gmx::SimulationSignaller* signalCoordinator, const matrix lastbox, - int* totalNumberOfBondedInteractions, gmx_bool* bSumEkinhOld, int flags); diff --git a/src/gromacs/mdlib/stat.cpp b/src/gromacs/mdlib/stat.cpp index 5aac4a342a..83c5314f7f 100644 --- a/src/gromacs/mdlib/stat.cpp +++ b/src/gromacs/mdlib/stat.cpp @@ -146,7 +146,6 @@ void global_stat(const gmx_global_stat* gs, t_vcm* vcm, int nsig, real* sig, - int* totalNumberOfBondedInteractions, bool bSumEkinhOld, int flags) /* instead of current system, gmx_booleans for summing virial, kinetic energy, and other terms */ @@ -274,7 +273,10 @@ void global_stat(const gmx_global_stat* gs, if (checkNumberOfBondedInteractions) { - nb = cr->dd->nbonded_local; + GMX_RELEASE_ASSERT(DOMAINDECOMP(cr), + "No need to check number of bonded interactions when not using domain " + "decomposition"); + nb = numBondedInteractions(*cr->dd); inb = add_bind(rb, 1, &nb); } if (nsig > 0) @@ -375,7 +377,10 @@ void global_stat(const gmx_global_stat* gs, if (checkNumberOfBondedInteractions) { extract_bind(rb, inb, 1, &nb); - *totalNumberOfBondedInteractions = gmx::roundToInt(nb); + GMX_RELEASE_ASSERT(DOMAINDECOMP(cr), + "No need to check number of bonded interactions when not using domain " + "decomposition"); + setNumberOfBondedInteractionsOverAllDomains(*cr->dd, gmx::roundToInt(nb)); } if (nsig > 0) diff --git a/src/gromacs/mdlib/stat.h b/src/gromacs/mdlib/stat.h index ad7c7657e7..850aaf3ead 100644 --- a/src/gromacs/mdlib/stat.h +++ b/src/gromacs/mdlib/stat.h @@ -74,7 +74,6 @@ void global_stat(const gmx_global_stat* gs, t_vcm* vcm, int nsig, real* sig, - int* totalNumberOfBondedInteractions, bool bSumEkinhOld, int flags); diff --git a/src/gromacs/mdlib/update_vv.cpp b/src/gromacs/mdlib/update_vv.cpp index aec700bce9..76334de0c5 100644 --- a/src/gromacs/mdlib/update_vv.cpp +++ b/src/gromacs/mdlib/update_vv.cpp @@ -45,7 +45,7 @@ #include #include -#include "gromacs/domdec/partition.h" +#include "gromacs/domdec/domdec.h" #include "gromacs/gmxlib/nrnb.h" #include "gromacs/math/units.h" #include "gromacs/math/vec.h" @@ -70,45 +70,44 @@ #include "gromacs/timing/wallcycle.h" #include "gromacs/topology/topology.h" -void integrateVVFirstStep(int64_t step, - bool bFirstStep, - bool bInitStep, - gmx::StartingBehavior startingBehavior, - int nstglobalcomm, - const t_inputrec* ir, - t_forcerec* fr, - t_commrec* cr, - t_state* state, - t_mdatoms* mdatoms, - const t_fcdata& fcdata, - t_extmass* MassQ, - t_vcm* vcm, - const gmx_mtop_t& top_global, - const gmx_localtop_t& top, - gmx_enerdata_t* enerd, - gmx_ekindata_t* ekind, - gmx_global_stat* gstat, - real* last_ekin, - bool bCalcVir, - tensor total_vir, - tensor shake_vir, - tensor force_vir, - tensor pres, - matrix M, - bool do_log, - bool do_ene, - bool bCalcEner, - bool bGStat, - bool bStopCM, - bool bTrotter, - bool bExchanged, - bool* bSumEkinhOld, - bool* shouldCheckNumberOfBondedInteractions, - real* saved_conserved_quantity, - gmx::ForceBuffers* f, - gmx::Update* upd, - gmx::Constraints* constr, - gmx::SimulationSignaller* nullSignaller, +void integrateVVFirstStep(int64_t step, + bool bFirstStep, + bool bInitStep, + gmx::StartingBehavior startingBehavior, + int nstglobalcomm, + const t_inputrec* ir, + t_forcerec* fr, + t_commrec* cr, + t_state* state, + t_mdatoms* mdatoms, + const t_fcdata& fcdata, + t_extmass* MassQ, + t_vcm* vcm, + const gmx_mtop_t& top_global, + const gmx_localtop_t& top, + gmx_enerdata_t* enerd, + gmx_ekindata_t* ekind, + gmx_global_stat* gstat, + real* last_ekin, + bool bCalcVir, + tensor total_vir, + tensor shake_vir, + tensor force_vir, + tensor pres, + matrix M, + bool do_log, + bool do_ene, + bool bCalcEner, + bool bGStat, + bool bStopCM, + bool bTrotter, + bool bExchanged, + bool* bSumEkinhOld, + real* saved_conserved_quantity, + gmx::ForceBuffers* f, + gmx::Update* upd, + gmx::Constraints* constr, + gmx::SimulationSignaller* nullSignaller, std::array, ettTSEQMAX> trotter_seq, t_nrnb* nrnb, const gmx::MDLogger& mdlog, @@ -165,7 +164,14 @@ void integrateVVFirstStep(int64_t step, if (bGStat || do_per_step(step - 1, nstglobalcomm)) { wallcycle_stop(wcycle, ewcUPDATE); - int totalNumberOfBondedInteractions = -1; + int cglo_flags = + ((bGStat ? CGLO_GSTAT : 0) | (bCalcEner ? CGLO_ENERGY : 0) + | (bTemp ? CGLO_TEMPERATURE : 0) | (bPres ? CGLO_PRESSURE : 0) + | (bPres ? CGLO_CONSTRAINT : 0) | (bStopCM ? CGLO_STOPCM : 0) | CGLO_SCALEEKIN); + if (DOMAINDECOMP(cr) && shouldCheckNumberOfBondedInteractions(*cr->dd)) + { + cglo_flags |= CGLO_CHECK_NUMBER_OF_BONDED_INTERACTIONS; + } compute_globals(gstat, cr, ir, @@ -186,14 +192,8 @@ void integrateVVFirstStep(int64_t step, (bCalcEner && constr != nullptr) ? constr->rmsdData() : gmx::ArrayRef{}, nullSignaller, state->box, - &totalNumberOfBondedInteractions, bSumEkinhOld, - (bGStat ? CGLO_GSTAT : 0) | (bCalcEner ? CGLO_ENERGY : 0) - | (bTemp ? CGLO_TEMPERATURE : 0) | (bPres ? CGLO_PRESSURE : 0) - | (bPres ? CGLO_CONSTRAINT : 0) | (bStopCM ? CGLO_STOPCM : 0) - | (*shouldCheckNumberOfBondedInteractions ? CGLO_CHECK_NUMBER_OF_BONDED_INTERACTIONS - : 0) - | CGLO_SCALEEKIN); + cglo_flags); /* explanation of above: a) We compute Ekin at the full time step if 1) we are using the AveVel Ekin, and it's not the @@ -201,14 +201,11 @@ void integrateVVFirstStep(int64_t step, time step kinetic energy for the pressure (always true now, since we want accurate statistics). b) If we are using EkinAveEkin for the kinetic energy for the temperature control, we still feed in EkinAveVel because it's needed for the pressure */ - checkNumberOfBondedInteractions(mdlog, - cr, - totalNumberOfBondedInteractions, - top_global, - &top, - makeConstArrayRef(state->x), - state->box, - shouldCheckNumberOfBondedInteractions); + if (DOMAINDECOMP(cr)) + { + checkNumberOfBondedInteractions( + mdlog, cr, top_global, &top, makeConstArrayRef(state->x), state->box); + } if (bStopCM) { process_and_stopcm_grp( @@ -268,7 +265,6 @@ void integrateVVFirstStep(int64_t step, gmx::ArrayRef{}, nullSignaller, state->box, - nullptr, bSumEkinhOld, CGLO_GSTAT | CGLO_TEMPERATURE); wallcycle_start(wcycle, ewcUPDATE); @@ -392,7 +388,6 @@ void integrateVVSecondStep(int64_t step, gmx::ArrayRef{}, nullSignaller, lastbox, - nullptr, bSumEkinhOld, (bGStat ? CGLO_GSTAT : 0) | CGLO_TEMPERATURE); wallcycle_start(wcycle, ewcUPDATE); diff --git a/src/gromacs/mdlib/update_vv.h b/src/gromacs/mdlib/update_vv.h index 71beb39db5..b1d7cf00ae 100644 --- a/src/gromacs/mdlib/update_vv.h +++ b/src/gromacs/mdlib/update_vv.h @@ -105,8 +105,6 @@ class Update; * \param[in] bTrotter Do trotter routines this step. * \param[in] bExchanged If this is a replica exchange step. * \param[out] bSumEkinhOld Old kinetic energies will need to be summed up. - * \param[out] shouldCheckNumberOfBondedInteractions If checks for missing bonded - * interactions will be needed. * \param[out] saved_conserved_quantity Place to store the conserved energy. * \param[in] f Force buffers. * \param[in] upd Update object. @@ -118,45 +116,44 @@ class Update; * \param[in] fplog Another logger. * \param[in] wcycle Wall-clock cycle counter. */ -void integrateVVFirstStep(int64_t step, - bool bFirstStep, - bool bInitStep, - gmx::StartingBehavior startingBehavior, - int nstglobalcomm, - const t_inputrec* ir, - t_forcerec* fr, - t_commrec* cr, - t_state* state, - t_mdatoms* mdatoms, - const t_fcdata& fcdata, - t_extmass* MassQ, - t_vcm* vcm, - const gmx_mtop_t& top_global, - const gmx_localtop_t& top, - gmx_enerdata_t* enerd, - gmx_ekindata_t* ekind, - gmx_global_stat* gstat, - real* last_ekin, - bool bCalcVir, - tensor total_vir, - tensor shake_vir, - tensor force_vir, - tensor pres, - matrix M, - bool do_log, - bool do_ene, - bool bCalcEner, - bool bGStat, - bool bStopCM, - bool bTrotter, - bool bExchanged, - bool* bSumEkinhOld, - bool* shouldCheckNumberOfBondedInteractions, - real* saved_conserved_quantity, - gmx::ForceBuffers* f, - gmx::Update* upd, - gmx::Constraints* constr, - gmx::SimulationSignaller* nullSignaller, +void integrateVVFirstStep(int64_t step, + bool bFirstStep, + bool bInitStep, + gmx::StartingBehavior startingBehavior, + int nstglobalcomm, + const t_inputrec* ir, + t_forcerec* fr, + t_commrec* cr, + t_state* state, + t_mdatoms* mdatoms, + const t_fcdata& fcdata, + t_extmass* MassQ, + t_vcm* vcm, + const gmx_mtop_t& top_global, + const gmx_localtop_t& top, + gmx_enerdata_t* enerd, + gmx_ekindata_t* ekind, + gmx_global_stat* gstat, + real* last_ekin, + bool bCalcVir, + tensor total_vir, + tensor shake_vir, + tensor force_vir, + tensor pres, + matrix M, + bool do_log, + bool do_ene, + bool bCalcEner, + bool bGStat, + bool bStopCM, + bool bTrotter, + bool bExchanged, + bool* bSumEkinhOld, + real* saved_conserved_quantity, + gmx::ForceBuffers* f, + gmx::Update* upd, + gmx::Constraints* constr, + gmx::SimulationSignaller* nullSignaller, std::array, ettTSEQMAX> trotter_seq, t_nrnb* nrnb, const gmx::MDLogger& mdlog, diff --git a/src/gromacs/mdrun/md.cpp b/src/gromacs/mdrun/md.cpp index 8c58a5bbc0..6e80825afe 100644 --- a/src/gromacs/mdrun/md.cpp +++ b/src/gromacs/mdrun/md.cpp @@ -200,15 +200,6 @@ void gmx::LegacySimulator::do_md() bool bInteractiveMDstep = false; - /* Domain decomposition could incorrectly miss a bonded - interaction, but checking for that requires a global - communication stage, which does not otherwise happen in DD - code. So we do that alongside the first global energy reduction - after a new DD is made. These variables handle whether the - check happens, and the result it returns. */ - bool shouldCheckNumberOfBondedInteractions = false; - int totalNumberOfBondedInteractions = -1; - SimulationSignals signals; // Most global communnication stages don't propagate mdrun // signals, and will use this object to achieve that. @@ -398,7 +389,6 @@ void gmx::LegacySimulator::do_md() nrnb, nullptr, FALSE); - shouldCheckNumberOfBondedInteractions = true; upd.setNumAtoms(state->natoms); } else @@ -657,6 +647,10 @@ void gmx::LegacySimulator::do_md() cglo_flags_iteration |= CGLO_STOPCM; cglo_flags_iteration &= ~CGLO_TEMPERATURE; } + if (DOMAINDECOMP(cr) && shouldCheckNumberOfBondedInteractions(*cr->dd) && cgloIteration == 0) + { + cglo_flags_iteration |= CGLO_CHECK_NUMBER_OF_BONDED_INTERACTIONS; + } compute_globals(gstat, cr, ir, @@ -677,11 +671,8 @@ void gmx::LegacySimulator::do_md() gmx::ArrayRef{}, &nullSignaller, state->box, - &totalNumberOfBondedInteractions, &bSumEkinhOld, - cglo_flags_iteration - | (shouldCheckNumberOfBondedInteractions ? CGLO_CHECK_NUMBER_OF_BONDED_INTERACTIONS - : 0)); + cglo_flags_iteration); if (cglo_flags_iteration & CGLO_STOPCM) { /* At initialization, do not pass x with acceleration-correction mode @@ -694,14 +685,11 @@ void gmx::LegacySimulator::do_md() inc_nrnb(nrnb, eNR_STOPCM, mdatoms->homenr); } } - checkNumberOfBondedInteractions(mdlog, - cr, - totalNumberOfBondedInteractions, - top_global, - &top, - makeConstArrayRef(state->x), - state->box, - &shouldCheckNumberOfBondedInteractions); + if (DOMAINDECOMP(cr)) + { + checkNumberOfBondedInteractions( + mdlog, cr, top_global, &top, makeConstArrayRef(state->x), state->box); + } if (ir->eI == IntegrationAlgorithm::VVAK) { /* a second call to get the half step temperature initialized as well */ @@ -730,7 +718,6 @@ void gmx::LegacySimulator::do_md() gmx::ArrayRef{}, &nullSignaller, state->box, - nullptr, &bSumEkinhOld, cglo_flags & ~CGLO_PRESSURE); } @@ -1021,7 +1008,6 @@ void gmx::LegacySimulator::do_md() nrnb, wcycle, do_verbose && !bPMETunePrinting); - shouldCheckNumberOfBondedInteractions = true; upd.setNumAtoms(state->natoms); } } @@ -1051,6 +1037,11 @@ void gmx::LegacySimulator::do_md() /* We need the kinetic energy at minus the half step for determining * the full step kinetic energy and possibly for T-coupling.*/ /* This may not be quite working correctly yet . . . . */ + int cglo_flags = CGLO_GSTAT | CGLO_TEMPERATURE; + if (DOMAINDECOMP(cr) && shouldCheckNumberOfBondedInteractions(*cr->dd)) + { + cglo_flags |= CGLO_CHECK_NUMBER_OF_BONDED_INTERACTIONS; + } compute_globals(gstat, cr, ir, @@ -1071,17 +1062,13 @@ void gmx::LegacySimulator::do_md() gmx::ArrayRef{}, &nullSignaller, state->box, - &totalNumberOfBondedInteractions, &bSumEkinhOld, - CGLO_GSTAT | CGLO_TEMPERATURE | CGLO_CHECK_NUMBER_OF_BONDED_INTERACTIONS); - checkNumberOfBondedInteractions(mdlog, - cr, - totalNumberOfBondedInteractions, - top_global, - &top, - makeConstArrayRef(state->x), - state->box, - &shouldCheckNumberOfBondedInteractions); + cglo_flags); + if (DOMAINDECOMP(cr)) + { + checkNumberOfBondedInteractions( + mdlog, cr, top_global, &top, makeConstArrayRef(state->x), state->box); + } } clear_mat(force_vir); @@ -1250,7 +1237,6 @@ void gmx::LegacySimulator::do_md() bTrotter, bExchanged, &bSumEkinhOld, - &shouldCheckNumberOfBondedInteractions, &saved_conserved_quantity, &f, &upd, @@ -1639,22 +1625,19 @@ void gmx::LegacySimulator::do_md() : gmx::ArrayRef{}, &signaller, lastbox, - &totalNumberOfBondedInteractions, &bSumEkinhOld, (bGStat ? CGLO_GSTAT : 0) | (!EI_VV(ir->eI) && bCalcEner ? CGLO_ENERGY : 0) | (!EI_VV(ir->eI) && bStopCM ? CGLO_STOPCM : 0) | (!EI_VV(ir->eI) ? CGLO_TEMPERATURE : 0) | (!EI_VV(ir->eI) ? CGLO_PRESSURE : 0) | CGLO_CONSTRAINT - | (shouldCheckNumberOfBondedInteractions ? CGLO_CHECK_NUMBER_OF_BONDED_INTERACTIONS - : 0)); - checkNumberOfBondedInteractions(mdlog, - cr, - totalNumberOfBondedInteractions, - top_global, - &top, - makeConstArrayRef(state->x), - state->box, - &shouldCheckNumberOfBondedInteractions); + | (DOMAINDECOMP(cr) && shouldCheckNumberOfBondedInteractions(*cr->dd) + ? CGLO_CHECK_NUMBER_OF_BONDED_INTERACTIONS + : 0)); + if (DOMAINDECOMP(cr)) + { + checkNumberOfBondedInteractions( + mdlog, cr, top_global, &top, makeConstArrayRef(state->x), state->box); + } if (!EI_VV(ir->eI) && bStopCM) { process_and_stopcm_grp( @@ -1907,7 +1890,6 @@ void gmx::LegacySimulator::do_md() nrnb, wcycle, FALSE); - shouldCheckNumberOfBondedInteractions = true; upd.setNumAtoms(state->natoms); } diff --git a/src/gromacs/mdrun/mimic.cpp b/src/gromacs/mdrun/mimic.cpp index 78239e1b07..ce23edc526 100644 --- a/src/gromacs/mdrun/mimic.cpp +++ b/src/gromacs/mdrun/mimic.cpp @@ -155,15 +155,6 @@ void gmx::LegacySimulator::do_mimic() double cycles; - /* Domain decomposition could incorrectly miss a bonded - interaction, but checking for that requires a global - communication stage, which does not otherwise happen in DD - code. So we do that alongside the first global energy reduction - after a new DD is made. These variables handle whether the - check happens, and the result it returns. */ - bool shouldCheckNumberOfBondedInteractions = false; - int totalNumberOfBondedInteractions = -1; - SimulationSignals signals; // Most global communnication stages don't propagate mdrun // signals, and will use this object to achieve that. @@ -316,7 +307,6 @@ void gmx::LegacySimulator::do_mimic() nrnb, nullptr, FALSE); - shouldCheckNumberOfBondedInteractions = true; } else { @@ -342,9 +332,11 @@ void gmx::LegacySimulator::do_mimic() } { - int cglo_flags = - (CGLO_GSTAT - | (shouldCheckNumberOfBondedInteractions ? CGLO_CHECK_NUMBER_OF_BONDED_INTERACTIONS : 0)); + int cglo_flags = CGLO_GSTAT; + if (DOMAINDECOMP(cr) && shouldCheckNumberOfBondedInteractions(*cr->dd)) + { + cglo_flags |= CGLO_CHECK_NUMBER_OF_BONDED_INTERACTIONS; + } bool bSumEkinhOld = false; t_vcm* vcm = nullptr; compute_globals(gstat, @@ -367,18 +359,14 @@ void gmx::LegacySimulator::do_mimic() gmx::ArrayRef{}, &nullSignaller, state->box, - &totalNumberOfBondedInteractions, &bSumEkinhOld, cglo_flags); + if (DOMAINDECOMP(cr)) + { + checkNumberOfBondedInteractions( + mdlog, cr, top_global, &top, makeConstArrayRef(state->x), state->box); + } } - checkNumberOfBondedInteractions(mdlog, - cr, - totalNumberOfBondedInteractions, - top_global, - &top, - makeConstArrayRef(state->x), - state->box, - &shouldCheckNumberOfBondedInteractions); if (MASTER(cr)) { @@ -502,7 +490,6 @@ void gmx::LegacySimulator::do_mimic() nrnb, wcycle, mdrunOptions.verbose); - shouldCheckNumberOfBondedInteractions = true; } if (MASTER(cr)) @@ -634,6 +621,11 @@ void gmx::LegacySimulator::do_mimic() t_vcm* vcm = nullptr; SimulationSignaller signaller(&signals, cr, ms, doInterSimSignal, doIntraSimSignal); + int cglo_flags = CGLO_GSTAT | CGLO_ENERGY; + if (DOMAINDECOMP(cr) && shouldCheckNumberOfBondedInteractions(*cr->dd)) + { + cglo_flags |= CGLO_CHECK_NUMBER_OF_BONDED_INTERACTIONS; + } compute_globals(gstat, cr, ir, @@ -654,19 +646,13 @@ void gmx::LegacySimulator::do_mimic() constr != nullptr ? constr->rmsdData() : gmx::ArrayRef{}, &signaller, state->box, - &totalNumberOfBondedInteractions, &bSumEkinhOld, - CGLO_GSTAT | CGLO_ENERGY - | (shouldCheckNumberOfBondedInteractions ? CGLO_CHECK_NUMBER_OF_BONDED_INTERACTIONS - : 0)); - checkNumberOfBondedInteractions(mdlog, - cr, - totalNumberOfBondedInteractions, - top_global, - &top, - makeConstArrayRef(state->x), - state->box, - &shouldCheckNumberOfBondedInteractions); + cglo_flags); + if (DOMAINDECOMP(cr)) + { + checkNumberOfBondedInteractions( + mdlog, cr, top_global, &top, makeConstArrayRef(state->x), state->box); + } } { diff --git a/src/gromacs/mdrun/minimize.cpp b/src/gromacs/mdrun/minimize.cpp index b8dc294a8c..8798e513cd 100644 --- a/src/gromacs/mdrun/minimize.cpp +++ b/src/gromacs/mdrun/minimize.cpp @@ -975,7 +975,6 @@ void EnergyEvaluator::run(em_state_t* ems, rvec mu_tot, tensor vir, tensor pres, nullptr, 1, &terminate, - nullptr, FALSE, CGLO_ENERGY | CGLO_PRESSURE | CGLO_CONSTRAINT); diff --git a/src/gromacs/mdrun/rerun.cpp b/src/gromacs/mdrun/rerun.cpp index 6ae83c4b3f..d3202d64fd 100644 --- a/src/gromacs/mdrun/rerun.cpp +++ b/src/gromacs/mdrun/rerun.cpp @@ -188,15 +188,6 @@ void gmx::LegacySimulator::do_rerun() double cycles; - /* Domain decomposition could incorrectly miss a bonded - interaction, but checking for that requires a global - communication stage, which does not otherwise happen in DD - code. So we do that alongside the first global energy reduction - after a new DD is made. These variables handle whether the - check happens, and the result it returns. */ - bool shouldCheckNumberOfBondedInteractions = false; - int totalNumberOfBondedInteractions = -1; - SimulationSignals signals; // Most global communnication stages don't propagate mdrun // signals, and will use this object to achieve that. @@ -360,7 +351,6 @@ void gmx::LegacySimulator::do_rerun() nrnb, nullptr, FALSE); - shouldCheckNumberOfBondedInteractions = true; } else { @@ -386,9 +376,11 @@ void gmx::LegacySimulator::do_rerun() } { - int cglo_flags = - (CGLO_GSTAT - | (shouldCheckNumberOfBondedInteractions ? CGLO_CHECK_NUMBER_OF_BONDED_INTERACTIONS : 0)); + int cglo_flags = CGLO_GSTAT; + if (DOMAINDECOMP(cr) && shouldCheckNumberOfBondedInteractions(*cr->dd)) + { + cglo_flags |= CGLO_CHECK_NUMBER_OF_BONDED_INTERACTIONS; + } bool bSumEkinhOld = false; t_vcm* vcm = nullptr; compute_globals(gstat, @@ -411,18 +403,14 @@ void gmx::LegacySimulator::do_rerun() gmx::ArrayRef{}, &nullSignaller, state->box, - &totalNumberOfBondedInteractions, &bSumEkinhOld, cglo_flags); + if (DOMAINDECOMP(cr)) + { + checkNumberOfBondedInteractions( + mdlog, cr, top_global, &top, makeConstArrayRef(state->x), state->box); + } } - checkNumberOfBondedInteractions(mdlog, - cr, - totalNumberOfBondedInteractions, - top_global, - &top, - makeConstArrayRef(state->x), - state->box, - &shouldCheckNumberOfBondedInteractions); if (MASTER(cr)) { @@ -612,7 +600,6 @@ void gmx::LegacySimulator::do_rerun() nrnb, wcycle, mdrunOptions.verbose); - shouldCheckNumberOfBondedInteractions = true; } if (MASTER(cr)) @@ -744,6 +731,11 @@ void gmx::LegacySimulator::do_rerun() t_vcm* vcm = nullptr; SimulationSignaller signaller(&signals, cr, ms, doInterSimSignal, doIntraSimSignal); + int cglo_flags = CGLO_GSTAT | CGLO_ENERGY; + if (DOMAINDECOMP(cr) && shouldCheckNumberOfBondedInteractions(*cr->dd)) + { + cglo_flags |= CGLO_CHECK_NUMBER_OF_BONDED_INTERACTIONS; + } compute_globals(gstat, cr, ir, @@ -764,19 +756,13 @@ void gmx::LegacySimulator::do_rerun() constr != nullptr ? constr->rmsdData() : gmx::ArrayRef{}, &signaller, state->box, - &totalNumberOfBondedInteractions, &bSumEkinhOld, - CGLO_GSTAT | CGLO_ENERGY - | (shouldCheckNumberOfBondedInteractions ? CGLO_CHECK_NUMBER_OF_BONDED_INTERACTIONS - : 0)); - checkNumberOfBondedInteractions(mdlog, - cr, - totalNumberOfBondedInteractions, - top_global, - &top, - makeConstArrayRef(state->x), - state->box, - &shouldCheckNumberOfBondedInteractions); + cglo_flags); + if (DOMAINDECOMP(cr)) + { + checkNumberOfBondedInteractions( + mdlog, cr, top_global, &top, makeConstArrayRef(state->x), state->box); + } } /* Note: this is OK, but there are some numerical precision issues with using the convergence of diff --git a/src/gromacs/modularsimulator/computeglobalselement.cpp b/src/gromacs/modularsimulator/computeglobalselement.cpp index 3f7ce609ad..fac796b438 100644 --- a/src/gromacs/modularsimulator/computeglobalselement.cpp +++ b/src/gromacs/modularsimulator/computeglobalselement.cpp @@ -43,7 +43,7 @@ #include "computeglobalselement.h" -#include "gromacs/domdec/partition.h" +#include "gromacs/domdec/domdec.h" #include "gromacs/gmxlib/network.h" #include "gromacs/gmxlib/nrnb.h" #include "gromacs/math/vec.h" @@ -90,8 +90,6 @@ ComputeGlobalsElement::ComputeGlobalsElement(StatePropagatorData* sta lastStep_(inputrec->nsteps + inputrec->init_step), initStep_(inputrec->init_step), nullSignaller_(std::make_unique(nullptr, nullptr, nullptr, false, false)), - totalNumberOfBondedInteractions_(0), - shouldCheckNumberOfBondedInteractions_(false), statePropagatorData_(statePropagatorData), energyData_(energyData), localTopology_(nullptr), @@ -283,33 +281,37 @@ void ComputeGlobalsElement::compute(gmx::Step step, auto lastbox = useLastBox ? statePropagatorData_->constPreviousBox() : statePropagatorData_->constBox(); - compute_globals( - gstat_, - cr_, - inputrec_, - fr_, - energyData_->ekindata(), - x, - v, - box, - mdAtoms_->mdatoms(), - nrnb_, - &vcm_, - step != -1 ? wcycle_ : nullptr, - energyData_->enerdata(), - energyData_->forceVirial(step), - energyData_->constraintVirial(step), - energyData_->totalVirial(step), - energyData_->pressure(step), - (((flags & CGLO_ENERGY) != 0) && constr_ != nullptr) ? constr_->rmsdData() - : gmx::ArrayRef{}, - signaller, - lastbox, - &totalNumberOfBondedInteractions_, - energyData_->needToSumEkinhOld(), - flags | (shouldCheckNumberOfBondedInteractions_ ? CGLO_CHECK_NUMBER_OF_BONDED_INTERACTIONS : 0)); - checkNumberOfBondedInteractions( - mdlog_, cr_, totalNumberOfBondedInteractions_, top_global_, localTopology_, x, box, &shouldCheckNumberOfBondedInteractions_); + if (DOMAINDECOMP(cr_) && shouldCheckNumberOfBondedInteractions(*cr_->dd)) + { + flags |= CGLO_CHECK_NUMBER_OF_BONDED_INTERACTIONS; + } + compute_globals(gstat_, + cr_, + inputrec_, + fr_, + energyData_->ekindata(), + x, + v, + box, + mdAtoms_->mdatoms(), + nrnb_, + &vcm_, + step != -1 ? wcycle_ : nullptr, + energyData_->enerdata(), + energyData_->forceVirial(step), + energyData_->constraintVirial(step), + energyData_->totalVirial(step), + energyData_->pressure(step), + (((flags & CGLO_ENERGY) != 0) && constr_ != nullptr) ? constr_->rmsdData() + : gmx::ArrayRef{}, + signaller, + lastbox, + energyData_->needToSumEkinhOld(), + flags); + if (DOMAINDECOMP(cr_)) + { + checkNumberOfBondedInteractions(mdlog_, cr_, top_global_, localTopology_, x, box); + } if (flags & CGLO_STOPCM && !isInit) { process_and_stopcm_grp(fplog_, &vcm_, *mdAtoms_->mdatoms(), x, v); @@ -317,18 +319,6 @@ void ComputeGlobalsElement::compute(gmx::Step step, } } -template -CheckBondedInteractionsCallback ComputeGlobalsElement::getCheckNumberOfBondedInteractionsCallback() -{ - return [this]() { needToCheckNumberOfBondedInteractions(); }; -} - -template -void ComputeGlobalsElement::needToCheckNumberOfBondedInteractions() -{ - shouldCheckNumberOfBondedInteractions_ = true; -} - template void ComputeGlobalsElement::setTopology(const gmx_localtop_t* top) { @@ -393,11 +383,6 @@ ISimulatorElement* ComputeGlobalsElement::get legacySimulatorData->top_global, legacySimulatorData->constr)); - // TODO: Remove this when DD can reduce bonded interactions independently (#3421) - auto* castedElement = static_cast*>(element); - globalCommunicationHelper->setCheckBondedInteractionsCallback( - castedElement->getCheckNumberOfBondedInteractionsCallback()); - return element; } @@ -439,13 +424,6 @@ ISimulatorElement* ComputeGlobalsElementfr, simulator->top_global, simulator->constr)); - - // TODO: Remove this when DD can reduce bonded interactions independently (#3421) - auto* castedElement = - static_cast*>( - vvComputeGlobalsElement); - globalCommunicationHelper->setCheckBondedInteractionsCallback( - castedElement->getCheckNumberOfBondedInteractionsCallback()); builderHelper->storeValue(key, vvComputeGlobalsElement); return vvComputeGlobalsElement; } diff --git a/src/gromacs/modularsimulator/domdechelper.cpp b/src/gromacs/modularsimulator/domdechelper.cpp index a55426cf09..e9af4e1fd3 100644 --- a/src/gromacs/modularsimulator/domdechelper.cpp +++ b/src/gromacs/modularsimulator/domdechelper.cpp @@ -56,25 +56,24 @@ namespace gmx { -DomDecHelper::DomDecHelper(bool isVerbose, - int verbosePrintInterval, - StatePropagatorData* statePropagatorData, - FreeEnergyPerturbationData* freeEnergyPerturbationData, - TopologyHolder* topologyHolder, - CheckBondedInteractionsCallback checkBondedInteractionsCallback, - int nstglobalcomm, - FILE* fplog, - t_commrec* cr, - const MDLogger& mdlog, - Constraints* constr, - const t_inputrec* inputrec, - MDAtoms* mdAtoms, - t_nrnb* nrnb, - gmx_wallcycle* wcycle, - t_forcerec* fr, - VirtualSitesHandler* vsite, - ImdSession* imdSession, - pull_t* pull_work) : +DomDecHelper::DomDecHelper(bool isVerbose, + int verbosePrintInterval, + StatePropagatorData* statePropagatorData, + FreeEnergyPerturbationData* freeEnergyPerturbationData, + TopologyHolder* topologyHolder, + int nstglobalcomm, + FILE* fplog, + t_commrec* cr, + const MDLogger& mdlog, + Constraints* constr, + const t_inputrec* inputrec, + MDAtoms* mdAtoms, + t_nrnb* nrnb, + gmx_wallcycle* wcycle, + t_forcerec* fr, + VirtualSitesHandler* vsite, + ImdSession* imdSession, + pull_t* pull_work) : nextNSStep_(-1), isVerbose_(isVerbose), verbosePrintInterval_(verbosePrintInterval), @@ -82,7 +81,6 @@ DomDecHelper::DomDecHelper(bool isVerbose, statePropagatorData_(statePropagatorData), freeEnergyPerturbationData_(freeEnergyPerturbationData), topologyHolder_(topologyHolder), - checkBondedInteractionsCallback_(std::move(checkBondedInteractionsCallback)), fplog_(fplog), cr_(cr), mdlog_(mdlog), @@ -180,7 +178,6 @@ void DomDecHelper::partitionSystem(bool verbose, wcycle, verbose); topologyHolder_->updateLocalTopology(); - checkBondedInteractionsCallback_(); statePropagatorData_->setLocalState(std::move(localState)); if (freeEnergyPerturbationData_) { diff --git a/src/gromacs/modularsimulator/domdechelper.h b/src/gromacs/modularsimulator/domdechelper.h index 41ca423492..8469cecc89 100644 --- a/src/gromacs/modularsimulator/domdechelper.h +++ b/src/gromacs/modularsimulator/domdechelper.h @@ -68,9 +68,6 @@ class VirtualSitesHandler; //! \addtogroup module_modularsimulator //! \{ -//! The function type allowing to request a check of the number of bonded interactions -typedef std::function CheckBondedInteractionsCallback; - /*! \internal * \brief Infrastructure element responsible for domain decomposition * @@ -88,25 +85,24 @@ class DomDecHelper final : public INeighborSearchSignallerClient { public: //! Constructor - DomDecHelper(bool isVerbose, - int verbosePrintInterval, - StatePropagatorData* statePropagatorData, - FreeEnergyPerturbationData* freeEnergyPerturbationData, - TopologyHolder* topologyHolder, - CheckBondedInteractionsCallback checkBondedInteractionsCallback, - int nstglobalcomm, - FILE* fplog, - t_commrec* cr, - const MDLogger& mdlog, - Constraints* constr, - const t_inputrec* inputrec, - MDAtoms* mdAtoms, - t_nrnb* nrnb, - gmx_wallcycle* wcycle, - t_forcerec* fr, - VirtualSitesHandler* vsite, - ImdSession* imdSession, - pull_t* pull_work); + DomDecHelper(bool isVerbose, + int verbosePrintInterval, + StatePropagatorData* statePropagatorData, + FreeEnergyPerturbationData* freeEnergyPerturbationData, + TopologyHolder* topologyHolder, + int nstglobalcomm, + FILE* fplog, + t_commrec* cr, + const MDLogger& mdlog, + Constraints* constr, + const t_inputrec* inputrec, + MDAtoms* mdAtoms, + t_nrnb* nrnb, + gmx_wallcycle* wcycle, + t_forcerec* fr, + VirtualSitesHandler* vsite, + ImdSession* imdSession, + pull_t* pull_work); /*! \brief Run domain decomposition * @@ -142,8 +138,6 @@ private: FreeEnergyPerturbationData* freeEnergyPerturbationData_; //! Pointer to the topology TopologyHolder* topologyHolder_; - //! Pointer to the ComputeGlobalsHelper object - to ask for # of bonded interaction checking - CheckBondedInteractionsCallback checkBondedInteractionsCallback_; //! Helper function unifying the DD partitioning calls in setup() and run() void partitionSystem(bool verbose, diff --git a/src/gromacs/modularsimulator/simulatoralgorithm.cpp b/src/gromacs/modularsimulator/simulatoralgorithm.cpp index df60829889..423c3e463c 100644 --- a/src/gromacs/modularsimulator/simulatoralgorithm.cpp +++ b/src/gromacs/modularsimulator/simulatoralgorithm.cpp @@ -551,7 +551,6 @@ ModularSimulatorAlgorithm ModularSimulatorAlgorithmBuilder::build() algorithm.statePropagatorData_.get(), algorithm.freeEnergyPerturbationData_.get(), algorithm.topologyHolder_.get(), - globalCommunicationHelper_.moveCheckBondedInteractionsCallback(), globalCommunicationHelper_.nstglobalcomm(), legacySimulatorData_->fplog, legacySimulatorData_->cr, @@ -738,21 +737,6 @@ SimulationSignals* GlobalCommunicationHelper::simulationSignals() return simulationSignals_; } -void GlobalCommunicationHelper::setCheckBondedInteractionsCallback(CheckBondedInteractionsCallback callback) -{ - checkBondedInteractionsCallback_ = std::move(callback); -} - -CheckBondedInteractionsCallback GlobalCommunicationHelper::moveCheckBondedInteractionsCallback() -{ - if (!checkBondedInteractionsCallback_) - { - throw SimulationAlgorithmSetupError( - "Requested CheckBondedInteractionsCallback before it was set."); - } - return *checkBondedInteractionsCallback_; -} - ModularSimulatorAlgorithmBuilderHelper::ModularSimulatorAlgorithmBuilderHelper( ModularSimulatorAlgorithmBuilder* builder) : builder_(builder) diff --git a/src/gromacs/modularsimulator/simulatoralgorithm.h b/src/gromacs/modularsimulator/simulatoralgorithm.h index 61b9737342..559818b1c2 100644 --- a/src/gromacs/modularsimulator/simulatoralgorithm.h +++ b/src/gromacs/modularsimulator/simulatoralgorithm.h @@ -292,18 +292,11 @@ public: //! Get a pointer to the signals vector [[nodiscard]] SimulationSignals* simulationSignals(); - //! Set the callback to check the number of bonded interactions - void setCheckBondedInteractionsCallback(CheckBondedInteractionsCallback callback); - //! Move the callback to check the number of bonded interactions - [[nodiscard]] CheckBondedInteractionsCallback moveCheckBondedInteractionsCallback(); - private: //! Compute globals communication period const int nstglobalcomm_; //! Signal vector (used by stop / reset / checkpointing signaller) SimulationSignals* simulationSignals_; - //! Callback to check the number of bonded interactions - std::optional checkBondedInteractionsCallback_; }; class ModularSimulatorAlgorithmBuilder; -- 2.22.0