Skip to content

Correctness testing with TOML#1682

Merged
rprospero merged 12 commits into
developfrom
TOML_Full_Test
Nov 3, 2023
Merged

Correctness testing with TOML#1682
rprospero merged 12 commits into
developfrom
TOML_Full_Test

Conversation

@rprospero
Copy link
Copy Markdown
Contributor

Now that TOML properly performs the round trip, we need to test for correctness. This PR performs the following changes

  1. For each test, we convert the input file to TOML and then run the tests on the imported TOML. This confirms that the TOML still produces the correct results
    a. We still perform the round trip testing to prevent regressions
    b. Tests tagged with TomlFailure only run the original input file unless the toml testing flag is activated. This marks the 17 tests currently failing under TOML
  2. The TOML serialisation and deserialisation no longer throws an exception when the TOML testing flag is inactive, since we are now using these procedures during the normal testing builds.
  3. We now perform proper TOML serialisation for ForceChargeSource and AutoChargeSource in the pair potentials. This ten line fix saves flagging the fifty tests that would fail without it.

@rprospero rprospero added the TOML Deals with the conversion of custom file formats into TOML label Oct 26, 2023
@rprospero rprospero requested a review from trisyoungs October 26, 2023 13:15
Copy link
Copy Markdown
Member

@trisyoungs trisyoungs left a comment

Choose a reason for hiding this comment

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

All looks good. Those failing tests are somewhat annoying, though!

Couple of comments, nothing major.

Comment thread tests/testData.h Outdated
Comment on lines +92 to +93
CoreData other_;
Dissolve trial_{other_};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These names are perhaps a bit too generic! I might suggest:

Suggested change
CoreData other_;
Dissolve trial_{other_};
CoreData otherCoreData_;
Dissolve otherDissolve_{otherCoreData_};

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've gone ahead and renamed them otherCoreData and otherDissolve. I dropped the trailing underscore because these aren't members of the class, but just variables in the function.

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,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we still need the SerializablePairPotential class? It feels like its just wrapping functionality that could be added into Dissolve::serialise() and Dissolve::deserialise()...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree that we need to review SerializablePairPotential, as I'm increasingly unhappy with the design. However, I'm not sure that Dissolve is the right place for it, either. I'll add an issue for it and we can review that before the final TOML addition.

Comment thread src/classes/serializablePairPotential.cpp Outdated
@rprospero rprospero merged commit b8635f5 into develop Nov 3, 2023
@rprospero rprospero deleted the TOML_Full_Test branch November 3, 2023 10:15
rprospero added a commit that referenced this pull request Mar 11, 2024
rprospero added a commit that referenced this pull request Apr 8, 2024
rprospero added a commit that referenced this pull request Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

TOML Deals with the conversion of custom file formats into TOML

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants