Skip to content

Commit

Permalink
Correctness testing with TOML (#1682)
Browse files Browse the repository at this point in the history
  • Loading branch information
@rprospero
rprospero authored Nov 3, 2023
1 parent 7147bd4 commit b8635f5
Show file tree
Hide file tree
Showing 7 changed files with 93 additions and 60 deletions.
18 changes: 13 additions & 5 deletions src/classes/serializablePairPotential.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@

#include "classes/serializablePairPotential.h"

SerializablePairPotential::SerializablePairPotential(double &range, double &delta, bool &source,
std::vector<std::shared_ptr<AtomType>> &types)
: range_(range), delta_(delta), atomTypeChargeSource_(source), atomTypes_(types),
coulombTruncationScheme_(PairPotential::coulombTruncationScheme_),
SerializablePairPotential::SerializablePairPotential(double &range, double &delta, bool &source, bool &forceCharge,
bool &autoCharge, std::vector<std::shared_ptr<AtomType>> &types)
: range_(range), delta_(delta), atomTypeChargeSource_(source), atomTypes_(types), forceCharge_(forceCharge),
autoCharge_(autoCharge), coulombTruncationScheme_(PairPotential::coulombTruncationScheme_),
shortRangeTruncationScheme_(PairPotential::shortRangeTruncationScheme_){};

double &SerializablePairPotential::range() { return range_; }
Expand Down Expand Up @@ -43,9 +43,15 @@ SerialisedValue SerializablePairPotential::serialise() const
SerialisedValue pairPotentials = {
{"range", range_},
{"delta", delta_},
{"includeCoulomb", atomTypeChargeSource_},
{"autoChargeSource", autoCharge_},
{"coulombTruncation", PairPotential::coulombTruncationSchemes().serialise(coulombTruncationScheme_)},
{"shortRangeTruncation", PairPotential::shortRangeTruncationSchemes().serialise(shortRangeTruncationScheme_)}};
if (!autoCharge_)
pairPotentials["autoChargeSource"] = false;
if (forceCharge_)
pairPotentials["forceChargeSource"] = true;
if (atomTypeChargeSource_)
pairPotentials["includeCoulomb"] = true;
for (auto &atomType : atomTypes_)
pairPotentials["atomTypes"][atomType->name().data()] = atomType->serialise();
return pairPotentials;
Expand All @@ -57,6 +63,8 @@ void SerializablePairPotential::deserialise(const SerialisedValue &node)
range_ = toml::find_or<double>(node, "range", 15.0);
delta_ = toml::find_or<double>(node, "delta", 0.005);
atomTypeChargeSource_ = toml::find_or<bool>(node, "includeCoulomb", false);
forceCharge_ = toml::find_or<bool>(node, "forceChargeSource", false);
autoCharge_ = toml::find_or<bool>(node, "autoChargeSource", true);

coulombTruncationScheme_ =
PairPotential::coulombTruncationSchemes().deserialise(toml::find_or<std::string>(node, "coulombTruncation", "Shifted"));
Expand Down
5 changes: 3 additions & 2 deletions src/classes/serializablePairPotential.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class SerializablePairPotential : public Serialisable<>
private:
double &range_;
double &delta_;
bool &atomTypeChargeSource_;
bool &atomTypeChargeSource_, &forceCharge_, &autoCharge_;

std::vector<std::shared_ptr<AtomType>> &atomTypes_;

Expand All @@ -24,7 +24,8 @@ class SerializablePairPotential : public Serialisable<>

// AtomTypes
public:
SerializablePairPotential(double &range, double &delta, bool &source, std::vector<std::shared_ptr<AtomType>> &types);
SerializablePairPotential(double &range, double &delta, bool &source, bool &forceCharge, bool &autoCharge,
std::vector<std::shared_ptr<AtomType>> &types);

double &range();
const double &range() const;
Expand Down
4 changes: 2 additions & 2 deletions src/main/dissolve.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
#include "classes/species.h"

Dissolve::Dissolve(CoreData &coreData)
: coreData_(coreData),
serializablePairPotential_(pairPotentialRange_, pairPotentialDelta_, atomTypeChargeSource_, coreData_.atomTypes())
: coreData_(coreData), serializablePairPotential_(pairPotentialRange_, pairPotentialDelta_, atomTypeChargeSource_,
forceChargeSource_, automaticChargeSource_, coreData_.atomTypes())
{
// Set core simulation variables
restartFileFrequency_ = 10;
Expand Down
13 changes: 0 additions & 13 deletions src/main/io.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,6 @@ SerialisedValue Dissolve::serialise() const
{
SerialisedValue root;

// If TOML is disabled,
if constexpr (!toml_testing_flag)
{
Messenger::error("This build does not support TOML.");
return root;
}

if (!coreData_.masterBonds().empty() || !coreData_.masterAngles().empty() || !coreData_.masterTorsions().empty() ||
!coreData_.masterImpropers().empty())
root["master"] = coreData_.serialiseMaster();
Expand All @@ -152,12 +145,6 @@ SerialisedValue Dissolve::serialise() const
// Read values from a serialisable value
void Dissolve::deserialise(const SerialisedValue &node)
{
// If TOML is disabled,
if constexpr (!toml_testing_flag)
{
Messenger::error("This build does not support TOML.");
return;
}

if (node.contains("pairPotentials"))
{
Expand Down
10 changes: 5 additions & 5 deletions tests/modules/epsr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ class EPSRModuleTest : public ::testing::Test

TEST_F(EPSRModuleTest, Water3NInpA)
{
ASSERT_NO_THROW(systemTest.setUp("dissolve/input/epsr-water-inpa.txt"));
ASSERT_NO_THROW(systemTest.setUp<TomlFailure>("dissolve/input/epsr-water-inpa.txt"));
ASSERT_TRUE(systemTest.dissolve().iterate(1));

// Estimated Partials
Expand Down Expand Up @@ -107,7 +107,7 @@ TEST_F(EPSRModuleTest, Water3NInpA)

TEST_F(EPSRModuleTest, Water3NX)
{
ASSERT_NO_THROW(systemTest.setUp("dissolve/input/epsr-water-3n-x.txt"));
ASSERT_NO_THROW(systemTest.setUp<TomlFailure>("dissolve/input/epsr-water-3n-x.txt"));
ASSERT_TRUE(systemTest.dissolve().iterate(1));

// Test total neutron-weighted F(r)
Expand Down Expand Up @@ -138,7 +138,7 @@ TEST_F(EPSRModuleTest, Water3NX)

TEST_F(EPSRModuleTest, Benzene)
{
ASSERT_NO_THROW(systemTest.setUp("dissolve/input/epsr-benzene-3n.txt"));
ASSERT_NO_THROW(systemTest.setUp<TomlFailure>("dissolve/input/epsr-benzene-3n.txt"));
ASSERT_TRUE(systemTest.dissolve().iterate(1));

// Test total neutron-weighted F(r)
Expand Down Expand Up @@ -200,7 +200,7 @@ TEST_F(EPSRModuleTest, BenzeneReadPCof) { ASSERT_NO_THROW(systemTest.setUp("diss

TEST_F(EPSRModuleTest, ScatteringMatrix)
{
ASSERT_NO_THROW(systemTest.setUp("dissolve/input/epsr-5datasets.txt"));
ASSERT_NO_THROW(systemTest.setUp<TomlFailure>("dissolve/input/epsr-5datasets.txt"));
ASSERT_TRUE(systemTest.dissolve().iterate(1));

// Find EPSR module
Expand All @@ -221,7 +221,7 @@ TEST_F(EPSRModuleTest, ScatteringMatrix)

TEST_F(EPSRModuleTest, DataWeighting)
{
ASSERT_NO_THROW(systemTest.setUp("dissolve/input/epsr-5datasets-weighted.txt"));
ASSERT_NO_THROW(systemTest.setUp<TomlFailure>("dissolve/input/epsr-5datasets-weighted.txt"));
ASSERT_TRUE(systemTest.dissolve().iterate(1));

// Find EPSR module
Expand Down
4 changes: 2 additions & 2 deletions tests/modules/siteRDF.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,8 @@ TEST_F(SiteRDFModuleTest, WaterDynamic)

TEST_F(SiteRDFModuleTest, WaterFragments)
{
ASSERT_NO_THROW(systemTest.setUp("dissolve/input/siteRDF-waterFragments.txt"));
ASSERT_TRUE(systemTest.iterateRestart(95));
ASSERT_NO_THROW(systemTest.setUp<TomlFailure>("dissolve/input/siteRDF-waterFragments.txt"));
ASSERT_TRUE(systemTest.iterateRestart<TomlFailure>(95));

// O-O RDF
EXPECT_TRUE(systemTest.checkData1D(
Expand Down
99 changes: 68 additions & 31 deletions tests/testData.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,47 +84,84 @@ class DissolveSystemTest
// Set up simulation ready for running, calling any additional setup function if already set
template <int flags = 0> void setUp(std::string_view inputFile)
{

dissolve_.clear();
if (!dissolve_.loadInput(inputFile))
throw(std::runtime_error(fmt::format("Input file '{}' failed to load correctly.\n", inputFile)));
if (rewriteCheck_)
if constexpr (Dissolve::toml_testing_flag || !(flags & TomlFailure))
{
auto newInput = fmt::format("{}/TestOutput_{}.{}.rewrite", DissolveSys::beforeLastChar(inputFile, '/'),
DissolveSys::afterLastChar(inputFile, '/'),
::testing::UnitTest::GetInstance()->current_test_info()->name());
if (!dissolve_.saveInput(newInput))
throw(std::runtime_error(fmt::format("Input file '{}' failed to rewrite correctly.\n", inputFile)));

dissolve_.clear();
if (!dissolve_.loadInput(newInput))
throw(std::runtime_error(fmt::format("Input file '{}' failed to reload correctly.\n", newInput)));
}
SerialisedValue toml;
{
CoreData otherCoreData;
Dissolve otherDissolve{otherCoreData};

if (!otherDissolve.loadInput(inputFile))
throw(std::runtime_error(fmt::format("Input file '{}' failed to load correctly.\n", inputFile)));
if (rewriteCheck_)
{
auto newInput = fmt::format("{}/TestOutput_{}.{}.rewrite", DissolveSys::beforeLastChar(inputFile, '/'),
DissolveSys::afterLastChar(inputFile, '/'),
::testing::UnitTest::GetInstance()->current_test_info()->name());
if (!otherDissolve.saveInput(newInput))
throw(std::runtime_error(fmt::format("Input file '{}' failed to rewrite correctly.\n", inputFile)));

otherDissolve.clear();
if (!otherDissolve.loadInput(newInput))
throw(std::runtime_error(fmt::format("Input file '{}' failed to reload correctly.\n", newInput)));
}

// Run any other additional setup functions
if (additionalSetUp_)
additionalSetUp_(otherDissolve, otherCoreData);

if (!otherDissolve.prepare())
throw(std::runtime_error("Failed to prepare simulation.\n"));

toml = otherDissolve.serialise();
}

dissolve_.deserialise(toml);
dissolve_.setInputFilename(std::string(inputFile));
auto repeat = dissolve_.serialise();

// Run any other additional setup functions
if (additionalSetUp_)
additionalSetUp_(dissolve_, coreData_);
// Run any other additional setup functions
if (additionalSetUp_)
additionalSetUp_(dissolve_, coreData_);

if (!dissolve_.prepare())
throw(std::runtime_error("Failed to prepare simulation.\n"));
if (!dissolve_.prepare())
throw(std::runtime_error("Failed to prepare simulation.\n"));

if (dissolve_.toml_testing_flag || !(flags & TomlFailure))
compareToml("", toml, repeat);
}
else
{
auto toml = dissolve_.serialise();
if (!dissolve_.loadInput(inputFile))
throw(std::runtime_error(fmt::format("Input file '{}' failed to load correctly.\n", inputFile)));
if (rewriteCheck_)
{
auto newInput = fmt::format("{}/TestOutput_{}.{}.rewrite", DissolveSys::beforeLastChar(inputFile, '/'),
DissolveSys::afterLastChar(inputFile, '/'),
::testing::UnitTest::GetInstance()->current_test_info()->name());
if (!dissolve_.saveInput(newInput))
throw(std::runtime_error(fmt::format("Input file '{}' failed to rewrite correctly.\n", inputFile)));

dissolve_.clear();
if (!dissolve_.loadInput(newInput))
throw(std::runtime_error(fmt::format("Input file '{}' failed to reload correctly.\n", newInput)));
}

CoreData other_;
Dissolve trial_{other_};
// Run any other additional setup functions
if (additionalSetUp_)
additionalSetUp_(dissolve_, coreData_);

trial_.deserialise(toml);
trial_.setInputFilename(std::string(inputFile));
auto repeat = trial_.serialise();
compareToml("", toml, repeat);
if (!dissolve_.prepare())
throw(std::runtime_error("Failed to prepare simulation.\n"));
}
}
void setUp(std::string_view inputFile, const std::function<void(Dissolve &D, CoreData &C)> &additionalSetUp,
bool known_toml_failure = false)

template <int flags = 0>
void setUp(std::string_view inputFile, const std::function<void(Dissolve &D, CoreData &C)> &additionalSetUp)
{
additionalSetUp_ = additionalSetUp;
setUp(inputFile);
setUp<flags>(inputFile);
}
// Load restart file
void loadRestart(std::string_view restartFile)
Expand All @@ -138,7 +175,7 @@ class DissolveSystemTest
*/
public:
// Iterate for set number of steps but chunked into smaller runs to test restart capability
bool iterateRestart(const int nIterations, const int chunkSize = 20)
template <int flags = 0> bool iterateRestart(const int nIterations, const int chunkSize = 20)
{
// Set the restart file frequency, and grab the input and restart filenames
dissolve_.setRestartFileFrequency(chunkSize);
Expand All @@ -160,7 +197,7 @@ class DissolveSystemTest
if (iterationsDone != nIterations)
{
fmt::print("Resetting at iteration {}...\n", iterationsDone);
setUp(inputFile);
setUp<flags>(inputFile);
loadRestart(restartFile);
}
}
Expand Down

1 comment on commit b8635f5

@github-actions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: b8635f5 Previous: 7147bd4 Ratio
BM_CalculateGR<ProblemType::atomic, Population::small, Method::CellsMethod>/iterations:5 16.367771200020798 ms/iter 7.144285000003947 ms/iter 2.29

This comment was automatically generated by workflow using github-action-benchmark.

CC: @disorderedmaterials/dissolve-devs

Please sign in to comment.