From f646e1d08f83d7fdbf739f32cfeb26a1ae24440d Mon Sep 17 00:00:00 2001 From: Pascal Merz Date: Fri, 25 Sep 2020 16:37:48 +0000 Subject: [PATCH] Replace FramesToCompare enum by MaxNumFrames This allows to explicitly chose a maximum number of frames to be tested, rather than only chosing to compare the first frame or all frames. It also implements to chose the number of frames for tests comparing to reference data rather than only when comparing simulation results. --- src/programs/mdrun/tests/comparison_helpers.h | 23 +++++++++--- src/programs/mdrun/tests/energycomparison.cpp | 35 +++++++++++++------ src/programs/mdrun/tests/energycomparison.h | 26 ++++++++++---- .../mdrun/tests/exactcontinuation.cpp | 2 +- src/programs/mdrun/tests/periodicactions.cpp | 2 +- src/programs/mdrun/tests/rerun.cpp | 2 +- src/programs/mdrun/tests/simulator.cpp | 2 +- .../mdrun/tests/simulatorcomparison.cpp | 2 +- .../mdrun/tests/trajectorycomparison.cpp | 4 +-- .../mdrun/tests/trajectorycomparison.h | 10 +++--- src/testutils/refdata.cpp | 8 +++++ src/testutils/refdata.h | 6 ++++ 12 files changed, 89 insertions(+), 33 deletions(-) diff --git a/src/programs/mdrun/tests/comparison_helpers.h b/src/programs/mdrun/tests/comparison_helpers.h index 0cde6c8dbe..d55c0a2274 100644 --- a/src/programs/mdrun/tests/comparison_helpers.h +++ b/src/programs/mdrun/tests/comparison_helpers.h @@ -49,12 +49,25 @@ namespace gmx namespace test { -//! Enumeration controlling which trajectory frames are compared -enum class FramesToCompare : int +/*! \internal + * \brief Named struct indicating the max number of frames to be compared */ +struct MaxNumFrames { - AllFrames, - OnlyFirstFrame, // e.g. because step-0 quantities are most useful to compare - Count + //! Explicit constructor + explicit MaxNumFrames(unsigned int maxFrame) : maxFrame_(maxFrame) {} + + //! Implicit conversion to int - struct can be used like underlying type + operator unsigned int() const { return maxFrame_; } + + //! Return a MaxNumFrames that will try to compare all frames + [[nodiscard]] static MaxNumFrames compareAllFrames() + { + return MaxNumFrames(std::numeric_limits::max()); + } + +private: + //! Internal value + const unsigned int maxFrame_; }; } // namespace test diff --git a/src/programs/mdrun/tests/energycomparison.cpp b/src/programs/mdrun/tests/energycomparison.cpp index f32520330c..cea5e9d021 100644 --- a/src/programs/mdrun/tests/energycomparison.cpp +++ b/src/programs/mdrun/tests/energycomparison.cpp @@ -78,9 +78,9 @@ EnergyTermsToCompare EnergyComparison::defaultEnergyTermsToCompare() }; EnergyComparison::EnergyComparison(const EnergyTermsToCompare& energyTermsToCompare, - FramesToCompare framesToCompare) : + MaxNumFrames maxNumFrames) : energyTermsToCompare_(energyTermsToCompare), - framesToCompare_(framesToCompare) + maxNumFrames_(maxNumFrames) { } @@ -97,7 +97,7 @@ std::vector EnergyComparison::getEnergyNames() const void EnergyComparison::operator()(const EnergyFrame& reference, const EnergyFrame& test) const { - if (framesToCompare_ == FramesToCompare::OnlyFirstFrame && firstFrameHasBeenCompared_) + if (numComparedFrames_ >= maxNumFrames_) { // Nothing should be compared return; @@ -122,18 +122,19 @@ void EnergyComparison::operator()(const EnergyFrame& reference, const EnergyFram ADD_FAILURE() << "Could not find energy component from reference frame in test frame"; } } - firstFrameHasBeenCompared_ = true; + numComparedFrames_++; } void checkEnergiesAgainstReferenceData(const std::string& energyFilename, const EnergyTermsToCompare& energyTermsToCompare, - TestReferenceChecker* checker) + TestReferenceChecker* checker, + MaxNumFrames maxNumEnergyFrames) { const bool thisRankChecks = (gmx_node_rank() == 0); if (thisRankChecks) { - EnergyComparison energyComparison(energyTermsToCompare, FramesToCompare::AllFrames); + EnergyComparison energyComparison(energyTermsToCompare, maxNumEnergyFrames); auto energyReader = openEnergyFileToReadTerms(energyFilename, energyComparison.getEnergyNames()); std::unordered_map checkers; @@ -150,11 +151,12 @@ void checkEnergiesAgainstReferenceData(const std::string& energyFilenam // frames with the same step number. But we need a unique // identifier so we match the intended reference data, so we // keep track of the number of the frame read from the file. - int frameNumber = 0; - while (energyReader->readNextFrame()) + unsigned int frameNumber = 0; + while (frameNumber < maxNumEnergyFrames && energyReader->readNextFrame()) { - const EnergyFrame& frame = energyReader->frame(); - const std::string frameName = frame.frameName() + " in frame " + toString(frameNumber); + const EnergyFrame& frame = energyReader->frame(); + const std::string frameName = + frame.frameName() + " in frame " + toString(static_cast(frameNumber)); SCOPED_TRACE("Comparing frame " + frameName); for (const auto& energyTermToCompare : energyTermsToCompare) @@ -167,6 +169,11 @@ void checkEnergiesAgainstReferenceData(const std::string& energyFilenam } ++frameNumber; } + if (frameNumber == maxNumEnergyFrames && energyReader->readNextFrame()) + { + // There would have been at least one more frame! + checker->disableUnusedEntriesCheck(); + } } else { @@ -174,5 +181,13 @@ void checkEnergiesAgainstReferenceData(const std::string& energyFilenam } } +void checkEnergiesAgainstReferenceData(const std::string& energyFilename, + const EnergyTermsToCompare& energyTermsToCompare, + TestReferenceChecker* checker) +{ + checkEnergiesAgainstReferenceData(energyFilename, energyTermsToCompare, checker, + MaxNumFrames::compareAllFrames()); +} + } // namespace test } // namespace gmx diff --git a/src/programs/mdrun/tests/energycomparison.h b/src/programs/mdrun/tests/energycomparison.h index b43d45692d..c33f66331b 100644 --- a/src/programs/mdrun/tests/energycomparison.h +++ b/src/programs/mdrun/tests/energycomparison.h @@ -71,7 +71,7 @@ public: //! Defaults for energy comparisons static EnergyTermsToCompare defaultEnergyTermsToCompare(); //! Constructor - EnergyComparison(const EnergyTermsToCompare& energyTermsToCompare, FramesToCompare framesToCompare); + EnergyComparison(const EnergyTermsToCompare& energyTermsToCompare, MaxNumFrames maxNumFrames); /*! \brief Return the names of energies that will be compared * * This function can be used to provide an input for @@ -94,15 +94,15 @@ public: private: //! Energy terms to match with given tolerances. EnergyTermsToCompare energyTermsToCompare_; - //! Which frames should be compared. - FramesToCompare framesToCompare_ = FramesToCompare::AllFrames; - /*! \brief Whether the first frame has been compared yet + //! How many frames should be compared. + MaxNumFrames maxNumFrames_ = MaxNumFrames::compareAllFrames(); + /*! \brief The number of frames that have been compared until now * * This field is mutable because the need to update the flag - * after the first frame is merely an implementation detail, + * when checking frames is merely an implementation detail, * rather than a proper change of internal state triggered * by the caller. */ - mutable bool firstFrameHasBeenCompared_ = false; + mutable unsigned int numComparedFrames_ = 0; }; /*! \brief Check a subset of the energies found in an energy file @@ -114,10 +114,24 @@ private: * \param[in] energyFilename The name of an energy file. * \param[in] energyTermsToCompare Set of energies to match at given tolerances. * \param[in] checker Root checker for reference data. + * \param[in] maxNumEnergyFrames The maximum number of frames to check * * \todo This is quite similar to the functionality used in PmeTest, * and we should consider reducing the duplication. */ +void checkEnergiesAgainstReferenceData(const std::string& energyFilename, + const EnergyTermsToCompare& energyTermsToCompare, + TestReferenceChecker* checker, + MaxNumFrames maxNumEnergyFrames); + +/*! + * \brief Check a subset of the energies found in an energy file + * against reference data. + * + * Convenience overload using all frames + * + * \see checkEnergiesAgainstReferenceData(const std::string&, const EnergyTermsToCompare&, TestReferenceChecker*, MaxNumFrames) + */ void checkEnergiesAgainstReferenceData(const std::string& energyFilename, const EnergyTermsToCompare& energyTermsToCompare, TestReferenceChecker* checker); diff --git a/src/programs/mdrun/tests/exactcontinuation.cpp b/src/programs/mdrun/tests/exactcontinuation.cpp index 737d98a389..ba2abb9b61 100644 --- a/src/programs/mdrun/tests/exactcontinuation.cpp +++ b/src/programs/mdrun/tests/exactcontinuation.cpp @@ -303,7 +303,7 @@ void runTest(TestFileManager* fileManager, // Build the functor that will compare energy frames on the chosen // energy terms. - EnergyComparison energyComparison(energyTermsToCompare, FramesToCompare::AllFrames); + EnergyComparison energyComparison(energyTermsToCompare, MaxNumFrames::compareAllFrames()); // Build the manager that will present matching pairs of frames to compare. // diff --git a/src/programs/mdrun/tests/periodicactions.cpp b/src/programs/mdrun/tests/periodicactions.cpp index 4f17a58795..41c69ab839 100644 --- a/src/programs/mdrun/tests/periodicactions.cpp +++ b/src/programs/mdrun/tests/periodicactions.cpp @@ -107,7 +107,7 @@ public: ReferenceFileNames referenceFileNames_ = { fileManager_.getTemporaryFilePath("reference.edr") }; //! Functor for energy comparison EnergyComparison energyComparison_{ EnergyComparison::defaultEnergyTermsToCompare(), - FramesToCompare::AllFrames }; + MaxNumFrames::compareAllFrames() }; //! Names of energies compared by energyComparison_ std::vector namesOfEnergiesToMatch_ = energyComparison_.getEnergyNames(); }; diff --git a/src/programs/mdrun/tests/rerun.cpp b/src/programs/mdrun/tests/rerun.cpp index 6f1946cfa3..e358b9cd1e 100644 --- a/src/programs/mdrun/tests/rerun.cpp +++ b/src/programs/mdrun/tests/rerun.cpp @@ -106,7 +106,7 @@ const TrajectoryFrameMatchSettings MdrunRerunTest::trajectoryMatchSettings = { ComparisonConditions::MustCompare, ComparisonConditions::NoComparison, ComparisonConditions::MustCompare, - FramesToCompare::AllFrames + MaxNumFrames::compareAllFrames() }; void executeRerunTest(TestFileManager* fileManager, diff --git a/src/programs/mdrun/tests/simulator.cpp b/src/programs/mdrun/tests/simulator.cpp index 6ed4f5f3c2..222b4ae41d 100644 --- a/src/programs/mdrun/tests/simulator.cpp +++ b/src/programs/mdrun/tests/simulator.cpp @@ -158,7 +158,7 @@ TEST_P(SimulatorComparisonTest, WithinTolerances) ComparisonConditions::MustCompare, ComparisonConditions::MustCompare, ComparisonConditions::MustCompare, - FramesToCompare::AllFrames }; + MaxNumFrames::compareAllFrames() }; TrajectoryTolerances trajectoryTolerances = TrajectoryComparison::s_defaultTrajectoryTolerances; if (simulationName != "argon12") { diff --git a/src/programs/mdrun/tests/simulatorcomparison.cpp b/src/programs/mdrun/tests/simulatorcomparison.cpp index 64a841f16e..c832d5ab9f 100644 --- a/src/programs/mdrun/tests/simulatorcomparison.cpp +++ b/src/programs/mdrun/tests/simulatorcomparison.cpp @@ -90,7 +90,7 @@ void compareEnergies(const std::string& edr1Name, { // Build the functor that will compare energy frames on the chosen // energy terms. - EnergyComparison energyComparison(energyTermsToCompare, FramesToCompare::AllFrames); + EnergyComparison energyComparison(energyTermsToCompare, MaxNumFrames::compareAllFrames()); // Build the manager that will present matching pairs of frames to compare. // diff --git a/src/programs/mdrun/tests/trajectorycomparison.cpp b/src/programs/mdrun/tests/trajectorycomparison.cpp index b4573bbeca..16a8a0980c 100644 --- a/src/programs/mdrun/tests/trajectorycomparison.cpp +++ b/src/programs/mdrun/tests/trajectorycomparison.cpp @@ -252,7 +252,7 @@ TrajectoryComparison::TrajectoryComparison(const TrajectoryFrameMatchSettings& m void TrajectoryComparison::operator()(const TrajectoryFrame& reference, const TrajectoryFrame& test) const { - if (matchSettings_.framesToCompare == FramesToCompare::OnlyFirstFrame && firstFrameHasBeenCompared_) + if (numComparedFrames_ >= matchSettings_.maxNumTrajectoryFrames) { // Nothing should be compared return; @@ -265,7 +265,7 @@ void TrajectoryComparison::operator()(const TrajectoryFrame& reference, const Tr compareCoordinates(reference, test, matchSettings_, tolerances_.coordinates); compareVelocities(reference, test, matchSettings_, tolerances_.velocities); compareForces(reference, test, matchSettings_, tolerances_.forces); - firstFrameHasBeenCompared_ = true; + numComparedFrames_++; } } // namespace test diff --git a/src/programs/mdrun/tests/trajectorycomparison.h b/src/programs/mdrun/tests/trajectorycomparison.h index 187045baa6..9e2847f738 100644 --- a/src/programs/mdrun/tests/trajectorycomparison.h +++ b/src/programs/mdrun/tests/trajectorycomparison.h @@ -99,8 +99,8 @@ struct TrajectoryFrameMatchSettings ComparisonConditions velocitiesComparison = ComparisonConditions::CompareIfBothFound; //! Whether forces must be compared. ComparisonConditions forcesComparison = ComparisonConditions::CompareIfBothFound; - //! Which frames will be compared - FramesToCompare framesToCompare = FramesToCompare::AllFrames; + //! How many frames will be compared. + MaxNumFrames maxNumTrajectoryFrames = MaxNumFrames::compareAllFrames(); }; /*! \internal @@ -134,13 +134,13 @@ private: TrajectoryFrameMatchSettings matchSettings_; //! Trajectory fields to match with given tolerances. TrajectoryTolerances tolerances_; - /*! \brief Whether the first frame has been compared yet + /*! \brief The number of frames that have been compared until now * * This field is mutable because the need to update the flag - * after the first frame is merely an implementation detail, + * when checking frames is merely an implementation detail, * rather than a proper change of internal state triggered * by the caller. */ - mutable bool firstFrameHasBeenCompared_ = false; + mutable unsigned int numComparedFrames_ = 0; }; } // namespace test diff --git a/src/testutils/refdata.cpp b/src/testutils/refdata.cpp index 7686090c7a..e2dc6ed6fd 100644 --- a/src/testutils/refdata.cpp +++ b/src/testutils/refdata.cpp @@ -756,6 +756,14 @@ void TestReferenceChecker::checkUnusedEntries() } } +void TestReferenceChecker::disableUnusedEntriesCheck() +{ + if (impl_->compareRootEntry_) + { + impl_->compareRootEntry_->setCheckedIncludingChildren(); + } +} + bool TestReferenceChecker::checkPresent(bool bPresent, const char* id) { diff --git a/src/testutils/refdata.h b/src/testutils/refdata.h index 3f681c5be3..21b8a2c960 100644 --- a/src/testutils/refdata.h +++ b/src/testutils/refdata.h @@ -302,6 +302,12 @@ public: */ void checkUnusedEntries(); + /*! \brief Disables checking for unused entries + * + * \see checkUnusedEntries() + */ + void disableUnusedEntriesCheck(); + /*! \brief * Checks whether a data item is present. * -- 2.22.0