From 4b260d3dcc8138ac2ef5b9f8a6371c84225d0172 Mon Sep 17 00:00:00 2001 From: Paul Bauer Date: Fri, 19 Mar 2021 11:11:49 +0000 Subject: [PATCH] Remove mdatoms from forceprovider call signature This makes it possible to test the forceprovider without having to build a partial mdatoms datastructure first. Needed for future refactoring of mdatoms. --- .../densityfittingamplitudelookup.cpp | 44 ++++++++++++------- .../densityfittingamplitudelookup.h | 11 ++--- .../densityfittingforceprovider.cpp | 6 +-- .../tests/densityfittingamplitudelookup.cpp | 24 ++++------ src/gromacs/applied_forces/electricfield.cpp | 11 +++-- .../applied_forces/tests/electricfield.cpp | 9 ++-- src/gromacs/mdlib/sim_util.cpp | 9 +++- src/gromacs/mdtypes/iforceprovider.h | 29 +++++++----- src/gromacs/restraint/restraintmdmodule.cpp | 12 ++--- 9 files changed, 86 insertions(+), 69 deletions(-) diff --git a/src/gromacs/applied_forces/densityfitting/densityfittingamplitudelookup.cpp b/src/gromacs/applied_forces/densityfitting/densityfittingamplitudelookup.cpp index 32dfd24aae..a3bcbc1fb2 100644 --- a/src/gromacs/applied_forces/densityfitting/densityfittingamplitudelookup.cpp +++ b/src/gromacs/applied_forces/densityfitting/densityfittingamplitudelookup.cpp @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2019,2020, by the GROMACS development team, led by + * Copyright (c) 2019,2020,2021, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -46,7 +46,6 @@ #include #include -#include "gromacs/mdtypes/mdatom.h" #include "gromacs/utility/arrayref.h" namespace gmx @@ -62,8 +61,10 @@ public: DensityFittingAmplitudeLookupImpl(const DensityFittingAmplitudeLookupImpl&) = default; virtual ~DensityFittingAmplitudeLookupImpl() = default; - virtual const std::vector& operator()(const t_mdatoms& atoms, ArrayRef localIndex) = 0; - virtual std::unique_ptr clone() = 0; + virtual const std::vector& operator()(ArrayRef /*chargeA*/, + ArrayRef /*massT*/, + ArrayRef localIndex) = 0; + virtual std::unique_ptr clone() = 0; }; namespace @@ -75,7 +76,9 @@ public: UnitAmplitudes(const UnitAmplitudes&) = default; ~UnitAmplitudes() override = default; std::unique_ptr clone() override; - const std::vector& operator()(const t_mdatoms& atoms, ArrayRef localIndex) override; + const std::vector& operator()(ArrayRef /*chargeA*/, + ArrayRef /*massT*/, + ArrayRef localIndex) override; private: std::vector amplitude_; @@ -86,7 +89,9 @@ std::unique_ptr UnitAmplitudes::clone() return std::make_unique(*this); }; -const std::vector& UnitAmplitudes::operator()(const t_mdatoms& /*atoms*/, ArrayRef localIndex) +const std::vector& UnitAmplitudes::operator()(ArrayRef /*chargeA*/, + ArrayRef /*massT*/, + ArrayRef localIndex) { if (amplitude_.size() != localIndex.size()) { @@ -103,7 +108,9 @@ public: ChargesAsAmplitudes(const ChargesAsAmplitudes&) = default; ~ChargesAsAmplitudes() override = default; std::unique_ptr clone() override; - const std::vector& operator()(const t_mdatoms& atoms, ArrayRef localIndex) override; + const std::vector& operator()(ArrayRef chargeA, + ArrayRef /*massT*/, + ArrayRef localIndex) override; private: std::vector amplitude_; @@ -114,7 +121,9 @@ std::unique_ptr ChargesAsAmplitudes::clone() return std::make_unique(*this); }; -const std::vector& ChargesAsAmplitudes::operator()(const t_mdatoms& atoms, ArrayRef localIndex) +const std::vector& ChargesAsAmplitudes::operator()(ArrayRef chargeA, + ArrayRef /*massT*/, + ArrayRef localIndex) { if (amplitude_.size() != localIndex.size()) { @@ -124,7 +133,7 @@ const std::vector& ChargesAsAmplitudes::operator()(const t_mdatoms& atoms, std::transform(std::begin(localIndex), std::end(localIndex), std::begin(amplitude_), - [&atoms](gmx::index index) { return atoms.chargeA[index]; }); + [&chargeA](gmx::index index) { return chargeA[index]; }); return amplitude_; } @@ -135,7 +144,9 @@ public: MassesAsAmplitudes(const MassesAsAmplitudes&) = default; ~MassesAsAmplitudes() override = default; std::unique_ptr clone() override; - const std::vector& operator()(const t_mdatoms& atoms, ArrayRef localIndex) override; + const std::vector& operator()(ArrayRef /*chargeA*/, + ArrayRef massT, + ArrayRef localIndex) override; private: std::vector amplitude_; @@ -146,7 +157,9 @@ std::unique_ptr MassesAsAmplitudes::clone() return std::make_unique(*this); }; -const std::vector& MassesAsAmplitudes::operator()(const t_mdatoms& atoms, ArrayRef localIndex) +const std::vector& MassesAsAmplitudes::operator()(ArrayRef /*chargeA*/, + ArrayRef massT, + ArrayRef localIndex) { if (amplitude_.size() != localIndex.size()) { @@ -156,7 +169,7 @@ const std::vector& MassesAsAmplitudes::operator()(const t_mdatoms& atoms, std::transform(std::begin(localIndex), std::end(localIndex), std::begin(amplitude_), - [&atoms](gmx::index index) { return atoms.massT[index]; }); + [&massT](gmx::index index) { return massT[index]; }); return amplitude_; } @@ -184,10 +197,11 @@ DensityFittingAmplitudeLookup::DensityFittingAmplitudeLookup(const DensityFittin } } -const std::vector& DensityFittingAmplitudeLookup::operator()(const t_mdatoms& atoms, - ArrayRef localIndex) +const std::vector& DensityFittingAmplitudeLookup::operator()(ArrayRef chargeA, + ArrayRef massT, + ArrayRef localIndex) { - return (*impl_)(atoms, localIndex); + return (*impl_)(chargeA, massT, localIndex); } DensityFittingAmplitudeLookup::~DensityFittingAmplitudeLookup() = default; diff --git a/src/gromacs/applied_forces/densityfitting/densityfittingamplitudelookup.h b/src/gromacs/applied_forces/densityfitting/densityfittingamplitudelookup.h index 86675bcec6..92b21f1e2b 100644 --- a/src/gromacs/applied_forces/densityfitting/densityfittingamplitudelookup.h +++ b/src/gromacs/applied_forces/densityfitting/densityfittingamplitudelookup.h @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2019,2020, by the GROMACS development team, led by + * Copyright (c) 2019,2020,2021, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -49,8 +49,6 @@ #include "gromacs/utility/enumerationhelpers.h" #include "gromacs/utility/real.h" -struct t_mdatoms; - namespace gmx { @@ -88,11 +86,14 @@ public: //! Move assignment DensityFittingAmplitudeLookup& operator=(DensityFittingAmplitudeLookup&& other) noexcept; /*! \brief Return the amplitudes for spreading atoms of a given local index. - * \param[in] atoms the atom information + * \param[in] chargeA Atom charges + * \param[in] massT Atom masses. * \param[in] localIndex the local atom indices * \returns amplitudes */ - const std::vector& operator()(const t_mdatoms& atoms, ArrayRef localIndex); + const std::vector& operator()(ArrayRef chargeA, + ArrayRef massT, + ArrayRef localIndex); private: std::unique_ptr impl_; diff --git a/src/gromacs/applied_forces/densityfitting/densityfittingforceprovider.cpp b/src/gromacs/applied_forces/densityfitting/densityfittingforceprovider.cpp index 4bd0a47df4..eec0e3c8b4 100644 --- a/src/gromacs/applied_forces/densityfitting/densityfittingforceprovider.cpp +++ b/src/gromacs/applied_forces/densityfitting/densityfittingforceprovider.cpp @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2019,2020, by the GROMACS development team, led by + * Copyright (c) 2019,2020,2021, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -298,8 +298,8 @@ void DensityFittingForceProvider::Impl::calculateForces(const ForceProviderInput // spread atoms on grid gaussTransform_.setZero(); - std::vector amplitudes = - amplitudeLookup_(forceProviderInput.mdatoms_, localAtomSet_.localIndex()); + std::vector amplitudes = amplitudeLookup_( + forceProviderInput.chargeA_, forceProviderInput.massT_, localAtomSet_.localIndex()); if (parameters_.normalizeDensities_) { diff --git a/src/gromacs/applied_forces/densityfitting/tests/densityfittingamplitudelookup.cpp b/src/gromacs/applied_forces/densityfitting/tests/densityfittingamplitudelookup.cpp index d0eeb3a9e3..7ffc527d22 100644 --- a/src/gromacs/applied_forces/densityfitting/tests/densityfittingamplitudelookup.cpp +++ b/src/gromacs/applied_forces/densityfitting/tests/densityfittingamplitudelookup.cpp @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2019,2020, by the GROMACS development team, led by + * Copyright (c) 2019,2020,2021, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -55,14 +55,6 @@ namespace gmx class DensityFittingAmplitudeLookupTest : public ::testing::Test { -public: - DensityFittingAmplitudeLookupTest() - { - atoms_.nr = numberOfAtoms_; - atoms_.massT = masses_.data(); - atoms_.chargeA = charges_.data(); - } - protected: int numberOfAtoms_ = 3; std::vector masses_ = { 2, 3, 4 }; @@ -74,7 +66,7 @@ protected: TEST_F(DensityFittingAmplitudeLookupTest, Unity) { DensityFittingAmplitudeLookup lookup(DensityFittingAmplitudeMethod::Unity); - const auto lookupResult = lookup(atoms_, lookupIndices_); + const auto lookupResult = lookup(charges_, masses_, lookupIndices_); EXPECT_EQ(lookupResult[0], 1); EXPECT_EQ(lookupResult[1], 1); } @@ -82,7 +74,7 @@ TEST_F(DensityFittingAmplitudeLookupTest, Unity) TEST_F(DensityFittingAmplitudeLookupTest, Charge) { DensityFittingAmplitudeLookup lookup(DensityFittingAmplitudeMethod::Charge); - const auto lookupResult = lookup(atoms_, lookupIndices_); + const auto lookupResult = lookup(charges_, masses_, lookupIndices_); EXPECT_EQ(lookupResult[0], 30); EXPECT_EQ(lookupResult[1], 40); } @@ -90,7 +82,7 @@ TEST_F(DensityFittingAmplitudeLookupTest, Charge) TEST_F(DensityFittingAmplitudeLookupTest, Masses) { DensityFittingAmplitudeLookup lookup(DensityFittingAmplitudeMethod::Mass); - const auto lookupResult = lookup(atoms_, lookupIndices_); + const auto lookupResult = lookup(charges_, masses_, lookupIndices_); EXPECT_EQ(lookupResult[0], 3); EXPECT_EQ(lookupResult[1], 4); } @@ -99,7 +91,7 @@ TEST_F(DensityFittingAmplitudeLookupTest, CanCopyAssign) { DensityFittingAmplitudeLookup lookup(DensityFittingAmplitudeMethod::Unity); DensityFittingAmplitudeLookup lookupCopied = lookup; - const auto lookupResult = lookupCopied(atoms_, lookupIndices_); + const auto lookupResult = lookupCopied(charges_, masses_, lookupIndices_); EXPECT_EQ(lookupResult[0], 1); EXPECT_EQ(lookupResult[1], 1); } @@ -108,7 +100,7 @@ TEST_F(DensityFittingAmplitudeLookupTest, CanCopyConstruct) { DensityFittingAmplitudeLookup lookup(DensityFittingAmplitudeMethod::Unity); DensityFittingAmplitudeLookup lookupCopied(lookup); - const auto lookupResult = lookupCopied(atoms_, lookupIndices_); + const auto lookupResult = lookupCopied(charges_, masses_, lookupIndices_); EXPECT_EQ(lookupResult[0], 1); EXPECT_EQ(lookupResult[1], 1); } @@ -117,7 +109,7 @@ TEST_F(DensityFittingAmplitudeLookupTest, CanMoveAssign) { DensityFittingAmplitudeLookup lookup(DensityFittingAmplitudeMethod::Unity); DensityFittingAmplitudeLookup lookupCopied = std::move(lookup); - const auto lookupResult = lookupCopied(atoms_, lookupIndices_); + const auto lookupResult = lookupCopied(charges_, masses_, lookupIndices_); EXPECT_EQ(lookupResult[0], 1); EXPECT_EQ(lookupResult[1], 1); } @@ -126,7 +118,7 @@ TEST_F(DensityFittingAmplitudeLookupTest, CanMoveConstruct) { DensityFittingAmplitudeLookup lookup(DensityFittingAmplitudeMethod::Unity); DensityFittingAmplitudeLookup lookupCopied(std::move(lookup)); - const auto lookupResult = lookupCopied(atoms_, lookupIndices_); + const auto lookupResult = lookupCopied(charges_, masses_, lookupIndices_); EXPECT_EQ(lookupResult[0], 1); EXPECT_EQ(lookupResult[1], 1); } diff --git a/src/gromacs/applied_forces/electricfield.cpp b/src/gromacs/applied_forces/electricfield.cpp index 6c4381747a..2140b62263 100644 --- a/src/gromacs/applied_forces/electricfield.cpp +++ b/src/gromacs/applied_forces/electricfield.cpp @@ -59,7 +59,6 @@ #include "gromacs/mdtypes/imdmodule.h" #include "gromacs/mdtypes/imdoutputprovider.h" #include "gromacs/mdtypes/imdpoptionprovider.h" -#include "gromacs/mdtypes/mdatom.h" #include "gromacs/options/basicoptions.h" #include "gromacs/options/ioptionscontainerwithsections.h" #include "gromacs/options/optionsection.h" @@ -303,13 +302,13 @@ void ElectricField::calculateForces(const ForceProviderInput& forceProviderInput { if (isActive()) { - const t_mdatoms& mdatoms = forceProviderInput.mdatoms_; - const double t = forceProviderInput.t_; - const t_commrec& cr = forceProviderInput.cr_; + const double t = forceProviderInput.t_; + const t_commrec& cr = forceProviderInput.cr_; // NOTE: The non-conservative electric field does not have a virial ArrayRef f = forceProviderOutput->forceWithVirial_.force_; + auto chargeA = forceProviderInput.chargeA_; for (int m = 0; (m < DIM); m++) { const real fieldStrength = gmx::c_fieldfac * field(m, t); @@ -317,10 +316,10 @@ void ElectricField::calculateForces(const ForceProviderInput& forceProviderInput if (fieldStrength != 0) { // TODO: Check parallellism - for (int i = 0; i < mdatoms.homenr; ++i) + for (int i = 0; i < forceProviderInput.homenr_; ++i) { // NOTE: Not correct with perturbed charges - f[i][m] += mdatoms.chargeA[i] * fieldStrength; + f[i][m] += chargeA[i] * fieldStrength; } } } diff --git a/src/gromacs/applied_forces/tests/electricfield.cpp b/src/gromacs/applied_forces/tests/electricfield.cpp index 8e19955ae3..fc514edae3 100644 --- a/src/gromacs/applied_forces/tests/electricfield.cpp +++ b/src/gromacs/applied_forces/tests/electricfield.cpp @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2015,2016,2017,2018,2019,2020, by the GROMACS development team, led by + * Copyright (c) 2015,2016,2017,2018,2019,2020,2021, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -106,13 +106,10 @@ public: } // Prepare a ForceProviderInput - t_mdatoms md; - std::vector chargeA{ 1 }; - md.homenr = ssize(chargeA); - md.chargeA = chargeA.data(); + std::vector chargeA{ 1 }; t_commrec cr; matrix boxDummy = { { 0, 0, 0 }, { 0, 0, 0 }, { 0, 0, 0 } }; - ForceProviderInput forceProviderInput({}, md, 0.0, boxDummy, cr); + ForceProviderInput forceProviderInput({}, ssize(chargeA), chargeA, {}, 0.0, boxDummy, cr); // Prepare a ForceProviderOutput PaddedVector f = { { 0, 0, 0 } }; diff --git a/src/gromacs/mdlib/sim_util.cpp b/src/gromacs/mdlib/sim_util.cpp index 5208ffc5c1..99dc4758c4 100644 --- a/src/gromacs/mdlib/sim_util.cpp +++ b/src/gromacs/mdlib/sim_util.cpp @@ -641,7 +641,14 @@ static void computeSpecialForces(FILE* fplog, */ if (stepWork.computeForces) { - gmx::ForceProviderInput forceProviderInput(x, *mdatoms, t, box, *cr); + gmx::ForceProviderInput forceProviderInput( + x, + mdatoms->homenr, + gmx::arrayRefFromArray(mdatoms->chargeA, mdatoms->homenr), + gmx::arrayRefFromArray(mdatoms->massT, mdatoms->homenr), + t, + box, + *cr); gmx::ForceProviderOutput forceProviderOutput(forceWithVirialMtsLevel0, enerd); /* Collect forces from modules */ diff --git a/src/gromacs/mdtypes/iforceprovider.h b/src/gromacs/mdtypes/iforceprovider.h index 748241617b..3006bf88e9 100644 --- a/src/gromacs/mdtypes/iforceprovider.h +++ b/src/gromacs/mdtypes/iforceprovider.h @@ -55,7 +55,6 @@ struct gmx_enerdata_t; struct t_commrec; struct t_forcerec; -struct t_mdatoms; namespace gmx { @@ -82,28 +81,36 @@ class ForceProviderInput public: /*! \brief Constructor assembles all necessary force provider input data * - * \param[in] x Atomic positions - * \param[in] cr Communication record structure - * \param[in] box The simulation box - * \param[in] time The current time in the simulation - * \param[in] mdatoms The atomic data + * \param[in] x Atomic positions. + * \param[in] homenr Number of atoms on the domain. + * \param[in] chargeA Atomic charges for atoms on the domain. + * \param[in] massT Atomic masses for atoms on the domain. + * \param[in] time The current time in the simulation. + * \param[in] box The simulation box. + * \param[in] cr Communication record structure. */ ForceProviderInput(ArrayRef x, - const t_mdatoms& mdatoms, + int homenr, + ArrayRef chargeA, + ArrayRef massT, double time, const matrix box, const t_commrec& cr) : x_(x), - mdatoms_(mdatoms), + homenr_(homenr), + chargeA_(chargeA), + massT_(massT), t_(time), cr_(cr) { copy_mat(box, box_); } - ArrayRef x_; //!< The atomic positions - const t_mdatoms& mdatoms_; //!< Atomic data - double t_; //!< The current time in the simulation + ArrayRef x_; //!< The atomic positions + int homenr_; + ArrayRef chargeA_; + ArrayRef massT_; + double t_; //!< The current time in the simulation matrix box_ = { { 0, 0, 0 }, { 0, 0, 0 }, { 0, 0, 0 } }; //!< The simulation box const t_commrec& cr_; //!< Communication record structure }; diff --git a/src/gromacs/restraint/restraintmdmodule.cpp b/src/gromacs/restraint/restraintmdmodule.cpp index b1dcb03d09..5ea9cdd07c 100644 --- a/src/gromacs/restraint/restraintmdmodule.cpp +++ b/src/gromacs/restraint/restraintmdmodule.cpp @@ -1,7 +1,7 @@ /* * This file is part of the GROMACS molecular simulation package. * - * Copyright (c) 2018,2019,2020, by the GROMACS development team, led by + * Copyright (c) 2018,2019,2020,2021, by the GROMACS development team, led by * Mark Abraham, David van der Spoel, Berk Hess, and Erik Lindahl, * and including many others, as listed in the AUTHORS file in the * top-level source directory and at http://www.gromacs.org. @@ -68,8 +68,8 @@ void RestraintForceProvider::calculateForces(const ForceProviderInput& forceProv { GMX_ASSERT(restraint_, "Restraint must be initialized."); - const auto& mdatoms = forceProviderInput.mdatoms_; - GMX_ASSERT(mdatoms.homenr >= 0, "number of home atoms must be non-negative."); + const int homenr = forceProviderInput.homenr_; + GMX_ASSERT(homenr >= 0, "number of home atoms must be non-negative."); const auto& box = forceProviderInput.box_; GMX_ASSERT(check_box(PbcType::Unset, box) == nullptr, "Invalid box."); @@ -80,7 +80,7 @@ void RestraintForceProvider::calculateForces(const ForceProviderInput& forceProv const auto& cr = forceProviderInput.cr_; const auto& t = forceProviderInput.t_; // Cooperatively get Cartesian coordinates for center of mass of each site - RVec r1 = sites_[0].centerOfMass(cr, static_cast(mdatoms.homenr), x, t); + RVec r1 = sites_[0].centerOfMass(cr, static_cast(homenr), x, t); // r2 is to be constructed as // r2 = (site[N] - site[N-1]) + (site_{N-1} - site_{N-2}) + ... + (site_2 - site_1) + site_1 // where the minimum image convention is applied to each path but not to the overall sum. @@ -96,8 +96,8 @@ void RestraintForceProvider::calculateForces(const ForceProviderInput& forceProv // a big molecule in a small box. for (size_t i = 0; i < sites_.size() - 1; ++i) { - RVec a = sites_[i].centerOfMass(cr, static_cast(mdatoms.homenr), x, t); - RVec b = sites_[i + 1].centerOfMass(cr, static_cast(mdatoms.homenr), x, t); + RVec a = sites_[i].centerOfMass(cr, static_cast(homenr), x, t); + RVec b = sites_[i + 1].centerOfMass(cr, static_cast(homenr), x, t); // dr = minimum_image_vector(b - a) pbc_dx(&pbc, b, a, dr); r2[0] += dr[0]; -- 2.22.0