From 0e448fce6beadf68abe1b29b0f69abc932bd9f44 Mon Sep 17 00:00:00 2001 From: Tristan Youngs Date: Fri, 26 Jun 2026 16:17:23 +0100 Subject: [PATCH 1/7] Start tinkering with a fixture for node testing with round-trip TOML plus data deserialisation. --- tests/testGraphFixture.h | 79 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 tests/testGraphFixture.h diff --git a/tests/testGraphFixture.h b/tests/testGraphFixture.h new file mode 100644 index 0000000000..78ceba98e4 --- /dev/null +++ b/tests/testGraphFixture.h @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +// Copyright (c) 2026 Team Dissolve and contributors + +#pragma once + +#include "tests/testGraph.h" + +namespace UnitTest +{ +class TestGraphFixture : public testing::Test +{ + public: + TestGraphFixture() = default; + ~TestGraphFixture() override = default; + + private: + // Serialised graph TOML + SerialisedValue graphTOML_; + + protected: + // Test graph + TestGraph testGraph_; + // Deserialised graph + DissolveGraph deserialisedGraph_; + // Current graph target + OptionalReferenceWrapper currentGraph_; + + private: + // Serialise the current graph to a TOML + bool serialiseGraphToTOML() + { + try + { + testGraph_.serialise("graph", graphTOML_); + } + catch (std::exception &ex) + { + std::cout << std::format("Failed to serialise graph TOML for test {}:\n", "TODO"); + std::cout << ex.what() << std::endl; + return false; + } + + return true; + } + + protected: + // Prepare any necessary test data + virtual void prepareTestData() = 0; + // Perform graph construction + virtual void constructGraph() = 0; + // Perform tests on generated data + virtual void performTests() = 0; + // Find specified node + template NodeClass *findNode(std::string nodeName) + { + EXPECT_TRUE(currentGraph_.has_value()); + auto *node = currentGraph_.value().get().findNode(nodeName); + EXPECT_TRUE(node); + return dynamic_cast(node); + } + + // Go + void go() + { + currentGraph_ = testGraph_; + + // Construct the graph + ASSERT_NO_THROW(constructGraph()); + // Run the tests + ASSERT_NO_THROW(performTests()); + // Serialise graph to TOML + ASSERT_TRUE(serialiseGraphToTOML()); + + // + currentGraph_ = deserialisedGraph_; + } + // +}; +}; // namespace UnitTest From cffeb116f511e78f32cad0ab4965548434f165a4 Mon Sep 17 00:00:00 2001 From: Tristan Youngs Date: Mon, 29 Jun 2026 09:42:32 +0100 Subject: [PATCH 2/7] Fix comment. --- src/nodes/graph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nodes/graph.cpp b/src/nodes/graph.cpp index 0c10f14966..d29fb5058b 100644 --- a/src/nodes/graph.cpp +++ b/src/nodes/graph.cpp @@ -283,7 +283,7 @@ void Graph::deserialise(const SerialisedValue &node) } /* - *Mermaid processing code + * Mermaid processing code */ // Node types that represent data sources From e8fa20f88a049c18aa165aec34b47e7d13f980ac Mon Sep 17 00:00:00 2001 From: Tristan Youngs Date: Mon, 29 Jun 2026 09:42:59 +0100 Subject: [PATCH 3/7] Test fixture with SDF node. --- tests/CMakeLists.txt | 2 +- tests/nodes/sdf.cpp | 167 ++++++++++++++++++++++++++------------- tests/testGraph.cpp | 2 +- tests/testGraphFixture.h | 18 ++++- 4 files changed, 128 insertions(+), 61 deletions(-) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ad9e4babe4..610a03c74c 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -62,7 +62,7 @@ function(dissolve_add_test) endfunction() -add_library(testing testing.cpp testGraph.cpp testing.h testGraph.h) +add_library(testing testing.cpp testGraph.cpp testing.h testGraph.h testGraphFixture.h) target_link_libraries(testing PRIVATE GTest::gtest_main) target_include_directories( testing PRIVATE ${PROJECT_SOURCE_DIR}/src ${PROJECT_BINARY_DIR}/src ${PROJECT_SOURCE_DIR} ${CONAN_INCLUDE_DIRS_GTEST} diff --git a/tests/nodes/sdf.cpp b/tests/nodes/sdf.cpp index 3344e5d1c0..f90f5a94ad 100644 --- a/tests/nodes/sdf.cpp +++ b/tests/nodes/sdf.cpp @@ -5,69 +5,122 @@ #include "nodes/importDLPUtilsPDens.h" #include "nodes/iterableGraph.h" #include "nodes/species.h" -#include "tests/testGraph.h" +#include "tests/testGraphFixture.h" namespace UnitTest { -TEST(SDFNodeTest, Water) +class SDFNodeWaterTest : public TestGraphFixture { - // Set up the test graph - TestGraph testGraph; - testGraph.createConfiguration("Box", {{"species/water-dlpoly.toml", 267}}, 0.1); + private: + Data3D referenceData_; - // Create trajectory iterator - auto iterator = testGraph.appendTrajectoryIterator("ImportXYZTrajectory", "dlpoly/water267-analysis/water-267-298K.xyz"); - EXPECT_TRUE(iterator); + protected: + // Prepare any necessary test data + void prepareTestData() override + { + ASSERT_TRUE(ImportDLPUtilsPDensNode::read(referenceData_, "dlpoly/water267-analysis/water-267-298K.11.pdens")); + } + // Perform graph construction + void constructGraph() override + { + testGraph_.createConfiguration("Box", {{"species/water-dlpoly.toml", 267}}, 0.1); - // Add the analysis module to the iterator - auto sdf = dynamic_cast(iterator->createNode("SDF")); - ASSERT_TRUE(sdf); - auto *water = testGraph.findNode("Water")->getOutputValue("Species"); - ASSERT_TRUE(water); - ASSERT_TRUE(sdf->setOption("SiteA", {{water->findSite("HMidpoint")}})); - ASSERT_TRUE(sdf->setOption("SiteB", {{water->findSite("HMidpoint")}})); - ASSERT_TRUE(sdf->setOption("RangeX", {-10.25, 10.25, 0.5})); - ASSERT_TRUE(sdf->setOption("RangeY", {-10.25, 10.25, 0.5})); - ASSERT_TRUE(sdf->setOption("RangeZ", {-10.25, 10.25, 0.5})); - ASSERT_TRUE(iterator->addEdge({testGraph.fetchHeadName(), "Configuration", "SDF", "Configuration"})); + // Create trajectory iterator + auto iterator = + testGraph_.appendTrajectoryIterator("ImportXYZTrajectory", "dlpoly/water267-analysis/water-267-298K.xyz"); + ASSERT_TRUE(iterator); + ASSERT_TRUE(iterator->setOption("N", 95)); - // Run from the iterator node explicitly - ASSERT_TRUE(iterator->setOption("N", 95)); - ASSERT_EQ(iterator->run(), NodeConstants::ProcessResult::Success); + // Add the analysis module to the iterator + auto sdf = dynamic_cast(iterator->createNode("SDF")); + ASSERT_TRUE(sdf); + auto *water = testGraph_.findNode("Water")->getOutputValue("Species"); + ASSERT_TRUE(water); + ASSERT_TRUE(sdf->setOption("SiteA", {{water->findSite("HMidpoint")}})); + ASSERT_TRUE(sdf->setOption("SiteB", {{water->findSite("HMidpoint")}})); + ASSERT_TRUE(sdf->setOption("RangeX", {-10.25, 10.25, 0.5})); + ASSERT_TRUE(sdf->setOption("RangeY", {-10.25, 10.25, 0.5})); + ASSERT_TRUE(sdf->setOption("RangeZ", {-10.25, 10.25, 0.5})); + ASSERT_TRUE(iterator->addEdge({testGraph_.fetchHeadName(), "Configuration", "SDF", "Configuration"})); + } + // Run the graph + void runGraph() + { + auto *iterator = findNode("Iterator"); + ASSERT_TRUE(iterator); + ASSERT_EQ(iterator->run(), NodeConstants::ProcessResult::Success); + } + // Perform tests on generated data + void performTests() + { + auto *iterator = findNode("Iterator"); + ASSERT_TRUE(iterator); + auto *sdf = dynamic_cast(iterator->findNode("SDF")); + ASSERT_TRUE(sdf); + EXPECT_TRUE(testData3D(sdf->sdf(), "SDF", referenceData_, "dlpoly/water267-analysis/water-267-298K.11.pdens", 0.13)); + } +}; - Data3D referenceData; - EXPECT_TRUE(ImportDLPUtilsPDensNode::read(referenceData, "dlpoly/water267-analysis/water-267-298K.11.pdens")); - EXPECT_TRUE(testData3D(sdf->sdf(), "SDF", referenceData, "dlpoly/water267-analysis/water-267-298K.11.pdens", 0.13)); -} - -TEST(SDFNodeTest, Benzene) -{ - // Set up the test graph - TestGraph testGraph; - testGraph.createConfiguration("Box", {{"species/benzene.toml", 181}}, {29.925089931000, 29.925089931000, 29.925089931000}); - - // Create trajectory iterator - auto iterator = testGraph.appendTrajectoryIterator("ImportXYZTrajectory", "dlpoly/benzene181/benzene181.xyz"); - EXPECT_TRUE(iterator); - - // Add the analysis module to the iterator - auto sdf = dynamic_cast(iterator->createNode("SDF")); - ASSERT_TRUE(sdf); - auto *benzene = testGraph.findNode("Benzene")->getOutputValue("Species"); - ASSERT_TRUE(benzene); - ASSERT_TRUE(sdf->setOption("SiteA", {{benzene->findSite("Ring")}})); - ASSERT_TRUE(sdf->setOption("SiteB", {{benzene->findSite("Ring")}})); - ASSERT_TRUE(sdf->setOption("RangeX", {-10.25, 10.25, 0.5})); - ASSERT_TRUE(sdf->setOption("RangeY", {-10.25, 10.25, 0.5})); - ASSERT_TRUE(sdf->setOption("RangeZ", {-10.25, 10.25, 0.5})); - ASSERT_TRUE(iterator->addEdge({testGraph.fetchHeadName(), "Configuration", "SDF", "Configuration"})); - - // Run from the iterator node explicitly - ASSERT_TRUE(iterator->setOption("N", 80)); - ASSERT_EQ(iterator->run(), NodeConstants::ProcessResult::Success); - - Data3D referenceData; - EXPECT_TRUE(ImportDLPUtilsPDensNode::read(referenceData, "dlpoly/benzene181/benzene181.11.pdens")); - EXPECT_TRUE(testData3D(sdf->sdf(), "SDF", referenceData, "dlpoly/benzene181/benzene181.11.pdens", 0.3)); -} +TEST_F(SDFNodeWaterTest, Water) { go(); } +// { +// // Set up the test graph +// TestGraph testGraph; +// testGraph.createConfiguration("Box", {{"species/water-dlpoly.toml", 267}}, 0.1); +// +// // Create trajectory iterator +// auto iterator = testGraph.appendTrajectoryIterator("ImportXYZTrajectory", "dlpoly/water267-analysis/water-267-298K.xyz"); +// EXPECT_TRUE(iterator); +// +// // Add the analysis module to the iterator +// auto sdf = dynamic_cast(iterator->createNode("SDF")); +// ASSERT_TRUE(sdf); +// auto *water = testGraph.findNode("Water")->getOutputValue("Species"); +// ASSERT_TRUE(water); +// ASSERT_TRUE(sdf->setOption("SiteA", {{water->findSite("HMidpoint")}})); +// ASSERT_TRUE(sdf->setOption("SiteB", {{water->findSite("HMidpoint")}})); +// ASSERT_TRUE(sdf->setOption("RangeX", {-10.25, 10.25, 0.5})); +// ASSERT_TRUE(sdf->setOption("RangeY", {-10.25, 10.25, 0.5})); +// ASSERT_TRUE(sdf->setOption("RangeZ", {-10.25, 10.25, 0.5})); +// ASSERT_TRUE(iterator->addEdge({testGraph.fetchHeadName(), "Configuration", "SDF", "Configuration"})); +// +// // Run from the iterator node explicitly +// ASSERT_TRUE(iterator->setOption("N", 95)); +// ASSERT_EQ(iterator->run(), NodeConstants::ProcessResult::Success); +// +// Data3D referenceData; +// EXPECT_TRUE(ImportDLPUtilsPDensNode::read(referenceData, "dlpoly/water267-analysis/water-267-298K.11.pdens")); +// EXPECT_TRUE(testData3D(sdf->sdf(), "SDF", referenceData, "dlpoly/water267-analysis/water-267-298K.11.pdens", 0.13)); +// } +// +// TEST(SDFNodeTest, Benzene) +// { +// // Set up the test graph +// TestGraph testGraph; +// testGraph.createConfiguration("Box", {{"species/benzene.toml", 181}}, +// {29.925089931000, 29.925089931000, 29.925089931000}); +// +// // Create trajectory iterator +// auto iterator = testGraph.appendTrajectoryIterator("ImportXYZTrajectory", "dlpoly/benzene181/benzene181.xyz"); +// EXPECT_TRUE(iterator); +// +// // Add the analysis module to the iterator +// auto sdf = dynamic_cast(iterator->createNode("SDF")); +// ASSERT_TRUE(sdf); +// auto *benzene = testGraph.findNode("Benzene")->getOutputValue("Species"); +// ASSERT_TRUE(benzene); +// ASSERT_TRUE(sdf->setOption("SiteA", {{benzene->findSite("Ring")}})); +// ASSERT_TRUE(sdf->setOption("SiteB", {{benzene->findSite("Ring")}})); +// ASSERT_TRUE(sdf->setOption("RangeX", {-10.25, 10.25, 0.5})); +// ASSERT_TRUE(sdf->setOption("RangeY", {-10.25, 10.25, 0.5})); +// ASSERT_TRUE(sdf->setOption("RangeZ", {-10.25, 10.25, 0.5})); +// ASSERT_TRUE(iterator->addEdge({testGraph.fetchHeadName(), "Configuration", "SDF", "Configuration"})); +// +// // Run from the iterator node explicitly +// ASSERT_TRUE(iterator->setOption("N", 80)); +// ASSERT_EQ(iterator->run(), NodeConstants::ProcessResult::Success); +// +// Data3D referenceData; +// EXPECT_TRUE(ImportDLPUtilsPDensNode::read(referenceData, "dlpoly/benzene181/benzene181.11.pdens")); +// EXPECT_TRUE(testData3D(sdf->sdf(), "SDF", referenceData, "dlpoly/benzene181/benzene181.11.pdens", 0.3)); +// } } // namespace UnitTest \ No newline at end of file diff --git a/tests/testGraph.cpp b/tests/testGraph.cpp index 57f0d3a5ca..09d867ec44 100644 --- a/tests/testGraph.cpp +++ b/tests/testGraph.cpp @@ -198,7 +198,7 @@ IterableGraph *TestGraph::appendTrajectoryIterator(std::string trajectoryImportN auto oldGraph = currentGraph_; // Add iterator node and make it the current graph - currentGraph_ = dynamic_cast(appendNode("Iterator", "Iterator")); + currentGraph_ = dynamic_cast(appendNode("Iterator")); EXPECT_TRUE(currentGraph_); head_ = nullptr; diff --git a/tests/testGraphFixture.h b/tests/testGraphFixture.h index 78ceba98e4..9ef6f707dd 100644 --- a/tests/testGraphFixture.h +++ b/tests/testGraphFixture.h @@ -48,6 +48,8 @@ class TestGraphFixture : public testing::Test virtual void prepareTestData() = 0; // Perform graph construction virtual void constructGraph() = 0; + // Run graph + virtual void runGraph() = 0; // Perform tests on generated data virtual void performTests() = 0; // Find specified node @@ -62,18 +64,30 @@ class TestGraphFixture : public testing::Test // Go void go() { + // Prepare test data + ASSERT_NO_THROW(prepareTestData()); + + // Set the initial graph target to the test graph currentGraph_ = testGraph_; // Construct the graph ASSERT_NO_THROW(constructGraph()); + // Run the graph + ASSERT_NO_THROW(runGraph()); // Run the tests ASSERT_NO_THROW(performTests()); // Serialise graph to TOML ASSERT_TRUE(serialiseGraphToTOML()); - // + // Switch to the deserialised graph target currentGraph_ = deserialisedGraph_; + + // Deserialise from the stored TOML + ASSERT_NO_THROW(deserialisedGraph_.deserialise(graphTOML_["graph"])); + // Run the graph + ASSERT_NO_THROW(runGraph()); + // Run the tests + ASSERT_NO_THROW(performTests()); } - // }; }; // namespace UnitTest From 9c349b7630b96783c4fd82ad5855ee8a3d1a0c9f Mon Sep 17 00:00:00 2001 From: Tristan Youngs Date: Tue, 30 Jun 2026 08:54:48 +0100 Subject: [PATCH 4/7] Exception handling in Node::deserialise(). --- src/nodes/node.cpp | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/nodes/node.cpp b/src/nodes/node.cpp index a566c7abbd..2058afdcc5 100644 --- a/src/nodes/node.cpp +++ b/src/nodes/node.cpp @@ -356,7 +356,14 @@ void Node::deserialise(const SerialisedValue &node) [this](const auto &k, const auto &v) { if (inputs_.contains(k)) - inputs_[k]->deserialise(v); + try + { + inputs_[k]->deserialise(v); + } + catch (std::exception &ex) + { + Messenger::exception("Error reading input {} in node {} ({}).", k, name(), ex.what()); + } else Messenger::exception("Node {} does not contain a parameter {}", name(), k); }); @@ -364,7 +371,14 @@ void Node::deserialise(const SerialisedValue &node) [this](const auto &k, const auto &v) { if (options_.contains(k)) - options_[k]->deserialise(v); + try + { + options_[k]->deserialise(v); + } + catch (std::exception &ex) + { + Messenger::exception("Error reading option {} in node {} ({}).", k, name(), ex.what()); + } else Messenger::exception("Node {} does not contain an option {}", name(), k); }); From a6e204d2fc55a93ce855a27769151b3b5de46216 Mon Sep 17 00:00:00 2001 From: Tristan Youngs Date: Tue, 30 Jun 2026 08:55:04 +0100 Subject: [PATCH 5/7] Direct type comparison in Parameter::deserialise(). --- src/nodes/parameter.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/nodes/parameter.h b/src/nodes/parameter.h index a925056362..1ee9451f18 100644 --- a/src/nodes/parameter.h +++ b/src/nodes/parameter.h @@ -606,23 +606,23 @@ template class SerialisableParameter : public Parameter) { DataClass proxy; // Fake T value to get the correct overload - Parameter::data_ = getEnumOptions(proxy).deserialise(node); + Parameter::data_ = getEnumOptions(proxy).enumeration(toml::find(node, "data")); } - else if constexpr (std::is_convertible>::value) + else if constexpr (std::is_same_v>) { if (node.contains("data")) Parameter::data_ = toml::find(node, "data"); else Parameter::data_ = {}; } - else if constexpr (std::is_convertible>::value) + else if constexpr (std::is_same_v>) { if (node.contains("data")) Parameter::data_ = toml::find(node, "data"); else Parameter::data_ = {}; } - else if constexpr (std::is_convertible>::value) + else if constexpr (std::is_same_v>) { if (node.contains("data")) Parameter::data_ = toml::find(node, "data"); From 0e2fe27155e11095d51144f5053ac7c10a3dd961 Mon Sep 17 00:00:00 2001 From: Tristan Youngs Date: Tue, 30 Jun 2026 08:55:13 +0100 Subject: [PATCH 6/7] Missed overrides. --- tests/nodes/sdf.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/nodes/sdf.cpp b/tests/nodes/sdf.cpp index f90f5a94ad..8367b9bcc8 100644 --- a/tests/nodes/sdf.cpp +++ b/tests/nodes/sdf.cpp @@ -44,14 +44,14 @@ class SDFNodeWaterTest : public TestGraphFixture ASSERT_TRUE(iterator->addEdge({testGraph_.fetchHeadName(), "Configuration", "SDF", "Configuration"})); } // Run the graph - void runGraph() + void runGraph() override { auto *iterator = findNode("Iterator"); ASSERT_TRUE(iterator); ASSERT_EQ(iterator->run(), NodeConstants::ProcessResult::Success); } // Perform tests on generated data - void performTests() + void performTests() override { auto *iterator = findNode("Iterator"); ASSERT_TRUE(iterator); From 87ad8f668dacc3b671b47918d9800779b1af905e Mon Sep 17 00:00:00 2001 From: RobBuchanan <106311829+RobBuchananCompPhys@users.noreply.github.com> Date: Mon, 27 Jul 2026 13:15:25 +0100 Subject: [PATCH 7/7] refactor: Graph & IterableGraph TOML roundtrip (#2559) --- src/nodes/edge.cpp | 78 ++++++++++++++++++++++--------- src/nodes/edge.h | 9 ++++ src/nodes/iterableGraph.cpp | 72 +++++++++++++++++++++------- src/nodes/iterableGraph.h | 11 +++++ tests/nodes/loop.cpp | 93 ++++++++++++++++++++++--------------- tests/nodes/subGraph.cpp | 83 +++++++++++++++++++++------------ tests/testing.cpp | 5 +- 7 files changed, 244 insertions(+), 107 deletions(-) diff --git a/src/nodes/edge.cpp b/src/nodes/edge.cpp index 616797bac3..29af27ee86 100644 --- a/src/nodes/edge.cpp +++ b/src/nodes/edge.cpp @@ -3,6 +3,7 @@ #include "nodes/edge.h" #include "nodes/graph.h" +#include "nodes/inputs.h" #include "nodes/loopBack.h" #include "nodes/outputs.h" @@ -31,6 +32,21 @@ class EdgeConstructor : public Edge // Create an edge from the supplied definition std::unique_ptr Edge::create(Graph *parent, const EdgeDefinition &definition) { + // Get target node + auto targetNode = parent->findNode(definition.targetNode); + if (!targetNode) + { + Messenger::error("Target node '{}' does not exist in the graph.\n", definition.targetNode); + return {}; + } + + // Disallow circular edges (mostly a check for Graph -> Graph connections) + if (targetNode == parent) + { + Messenger::error("Target node is graph '{}' and cannot be the owner of the edge.", definition.targetNode); + return {}; + } + // Get source node and output auto sourceNode = parent->findNode(definition.sourceNode); if (!sourceNode) @@ -38,11 +54,27 @@ std::unique_ptr Edge::create(Graph *parent, const EdgeDefinition &definiti Messenger::error("Source node '{}' does not exist in the graph.\n", definition.sourceNode); return {}; } + auto sourceOutput = sourceNode->findOutput(definition.sourceOutput); if (!sourceOutput) { - Messenger::error("Source node '{}' has no output parameter '{}'.\n", definition.sourceNode, definition.sourceOutput); - return {}; + // If the source node is a Graph's own Inputs node, we will create an edge on the fly - else, throw an error + if (!dynamic_cast(sourceNode)) + { + Messenger::error("Source node '{}' has no output parameter '{}'.\n", definition.sourceNode, + definition.sourceOutput); + return {}; + } + + // The target node is the parent Graph's own Inputs node, so create a parameter link from the mapped input to the + // targetInput + auto link = targetNode->findInput(definition.targetInput)->createParameterLink(definition.sourceOutput); + if (!parent->addProxyInput(link.inputParameter, link.outputParameter)) + { + Messenger::error("Failed to add mapped input '{}'.\n", definition.targetInput); + return {}; + } + sourceOutput = parent->proxyInputs().findOutput(definition.sourceOutput); } // Confirm that the source is actually an output @@ -53,21 +85,6 @@ std::unique_ptr Edge::create(Graph *parent, const EdgeDefinition &definiti return {}; } - // Get target node and input - auto targetNode = parent->findNode(definition.targetNode); - if (!targetNode) - { - Messenger::error("Target node '{}' does not exist in the graph.\n", definition.targetNode); - return {}; - } - - // Disallow circular edges (mostly a check for Graph -> Graph connections) - if (targetNode == parent) - { - Messenger::error("Target node is graph '{}' and cannot be the owner of the edge.", definition.targetNode); - return {}; - } - // We need to check carefully the target node, since we need to permit outside connections to the Graph object itself as // well as its Outputs node explicitly. std::shared_ptr targetInput{nullptr}; @@ -75,13 +92,19 @@ std::unique_ptr Edge::create(Graph *parent, const EdgeDefinition &definiti { // The target node is a Graph: create a parameter link from the sourceOutput and from it a mapped input auto graphNode = dynamic_cast(targetNode); - auto link = sourceOutput->createParameterLink(definition.targetInput); - if (!graphNode->addProxyInput(link.inputParameter, link.outputParameter)) + auto existingTargetInput = graphNode->findInput(definition.targetInput); + if (!existingTargetInput.get()) { - Messenger::error("Failed to add mapped input '{}'.\n", definition.targetInput); - return {}; + auto link = sourceOutput->createParameterLink(definition.targetInput); + if (!graphNode->addProxyInput(link.inputParameter, link.outputParameter)) + { + Messenger::error("Failed to add mapped input '{}'.\n", definition.targetInput); + return {}; + } + targetInput = link.inputParameter; } - targetInput = link.inputParameter; + else + targetInput = existingTargetInput; } else if (dynamic_cast(targetNode)) { @@ -288,3 +311,14 @@ void Edge::deserialise(const SerialisedValue &node) throw std::runtime_error("Cannot directly deserialise edges. Please contact the Dissolve development team if you are " "seeing this error - this is a bug and NOT your fault.\n"); } + +// Express as a serialisable value +void LoopEdge::serialise(std::string tag, SerialisedValue &target) const +{ + definition().serialise(tag, target); + target[tag]["targetNode"] = "LoopBacks"; + target[tag]["analogue"] = analogue_; +} + +// Read values from a serialisable value +void LoopEdge::deserialise(const SerialisedValue &node) { Edge::deserialise(node); } diff --git a/src/nodes/edge.h b/src/nodes/edge.h index 4d69360cf8..ba70352893 100644 --- a/src/nodes/edge.h +++ b/src/nodes/edge.h @@ -115,4 +115,13 @@ class LoopEdge : public Edge * */ ParameterBase *analogue_; + + /* + * Serialisation + */ + public: + // Express as a serialisable value + void serialise(std::string tag, SerialisedValue &target) const override; + // Read values from a serialisable value + void deserialise(const SerialisedValue &node) override; }; \ No newline at end of file diff --git a/src/nodes/iterableGraph.cpp b/src/nodes/iterableGraph.cpp index 6ae7583aa1..d9e9838f3f 100644 --- a/src/nodes/iterableGraph.cpp +++ b/src/nodes/iterableGraph.cpp @@ -85,6 +85,12 @@ LoopEdge *IterableGraph::findLoopEdge(const EdgeDefinition &definition) const return {}; } +// Add edge between nodes +bool IterableGraph::addLoopEdge(std::unique_ptr edge, std::string_view source) +{ + return addOutputLoopEdge(source, loopEdges_.emplace_back(LoopEdge::makeLoopEdge(edge.release(), proxyInputs())).get()); +} + // Add edge to node map Edge *IterableGraph::addOutputLoopEdge(std::string_view sourceOutput, Edge *edge) { @@ -128,23 +134,26 @@ Edge *IterableGraph::removeOutputLoopEdge(std::string_view sourceOutput, Edge *e // Add edge between nodes bool IterableGraph::addEdge(const EdgeDefinition &definition) { - if (dynamic_cast(parentGraph()->findNode(definition.sourceNode))) - setLoopBacks(); - else if (loopBacks_->findInput(definition.targetInput)) - { - auto edge = - Edge::create(this, {definition.sourceNode, definition.sourceOutput, definition.targetNode, definition.targetInput}); - if (!edge) - return false; - - loopEdges_.emplace_back(LoopEdge::makeLoopEdge(edge.release(), proxyInputs())); - - addOutputLoopEdge(definition.sourceOutput, loopEdges_.back().get()); - - return true; - } - - return Graph::addEdge(definition); + // Refresh the graph loopbacks + setLoopBacks(); + + // Check if the connection is invertible. + // Invertibility is satisfied when the source node (internal to the graph) can output to an existing loopback, + // which discounts any edge for which no loopbacks correspond to the target input, as well as the graphs own InputsNode. + auto nonInvertible = dynamic_cast(parentGraph()->findNode(definition.sourceNode)) || + !loopBacks_->findInput(definition.targetInput); + + // If not invertible, create and return a standard edge + if (nonInvertible) + return Graph::addEdge(definition); + + // Create loop edge + auto edge = + Edge::create(this, {definition.sourceNode, definition.sourceOutput, definition.targetNode, definition.targetInput}); + if (!edge) + return false; + + return addLoopEdge(std::move(edge), definition.sourceOutput); } // Remove edge between nodes @@ -183,3 +192,32 @@ NodeConstants::ProcessResult IterableGraph::process() return NodeConstants::ProcessResult::Success; } + +/* + * Serialisation + */ + +// Express as a serialisable value +void IterableGraph::serialise(std::string tag, SerialisedValue &target) const +{ + Graph::serialise(tag, target); + auto &result = target[tag]; + fromVector(loopEdges_, "loopEdges", result); +} + +// Read values from a serialisable value +void IterableGraph::deserialise(const SerialisedValue &node) +{ + Graph::deserialise(node); + toVector(node, "loopEdges", + [this](const auto &value) + { + auto definition = toml::get(value); + auto edge = Edge::create( + this, {definition.sourceNode, definition.sourceOutput, definition.targetNode, definition.targetInput}); + if (!edge) + return false; + + return addLoopEdge(std::move(edge), definition.sourceOutput); + }); +} diff --git a/src/nodes/iterableGraph.h b/src/nodes/iterableGraph.h index c8b51cc9df..a7882bddf1 100644 --- a/src/nodes/iterableGraph.h +++ b/src/nodes/iterableGraph.h @@ -46,6 +46,8 @@ class IterableGraph : public Graph void releaseLoopBack(const std::string &name); private: + // Add edge between nodes + bool addLoopEdge(std::unique_ptr edge, std::string_view source); // Add edge to node map Edge *addOutputLoopEdge(std::string_view sourceOutput, Edge *edge); // Remove edge from node map @@ -80,4 +82,13 @@ class IterableGraph : public Graph protected: // Perform processing NodeConstants::ProcessResult process() override; + + /* + * Serialisation + */ + public: + // Express as a serialisable value + void serialise(std::string tag, SerialisedValue &target) const override; + // Read values from a serialisable value + void deserialise(const SerialisedValue &node) override; }; diff --git a/tests/nodes/loop.cpp b/tests/nodes/loop.cpp index 21ccf88364..ca0102d0c4 100644 --- a/tests/nodes/loop.cpp +++ b/tests/nodes/loop.cpp @@ -7,6 +7,7 @@ #include "nodes/numberNode.h" #include "nodes/outputs.h" #include "nodes/registry.h" +#include "tests/testGraphFixture.h" #include namespace UnitTest @@ -37,40 +38,58 @@ class IterableGraphTest : public ::testing::Test // Create nodes i_ = dynamic_cast(root_.createNode("Number", "i")); - loop_ = dynamic_cast(root_.createNode("Iterator", "Iterator")); - x_ = dynamic_cast(loop_->createNode("Add", "x")); + loopGraph_ = dynamic_cast(root_.createNode("Iterator", "Iterator")); + x_ = dynamic_cast(loopGraph_->createNode("Add", "x")); y_ = dynamic_cast(root_.createNode("Add", "y")); ASSERT_TRUE(i_); ASSERT_TRUE(x_); ASSERT_TRUE(y_); - ASSERT_TRUE(loop_); + ASSERT_TRUE(loopGraph_); ASSERT_EQ(i_->name(), "i"); ASSERT_EQ(x_->name(), "x"); ASSERT_EQ(y_->name(), "y"); - ASSERT_EQ(loop_->name(), "Iterator"); + ASSERT_EQ(loopGraph_->name(), "Iterator"); // Create edge connections // - Number 'i' is a dynamic input to the IterableGraph - we'll call the input "I" EXPECT_TRUE(root_.addEdge({"i", "X", "Iterator", "I"})); // - Add 'x' takes the IterableGraph input "I" as its parameter "X" - EXPECT_TRUE(loop_->addEdge({"Inputs", "I", "x", "X"})); + EXPECT_TRUE(loopGraph_->addEdge({"Inputs", "I", "x", "X"})); // - Result from Add 'x' goes to graph output (which we will call "C") as well as loopback to "I" - EXPECT_TRUE(loop_->addEdge({"x", "Result", "Outputs", "C"})); - EXPECT_TRUE(loop_->addEdge({"x", "Result", "LoopBacks", "I"})); + EXPECT_TRUE(loopGraph_->addEdge({"x", "Result", "Outputs", "C"})); + EXPECT_TRUE(loopGraph_->addEdge({"x", "Result", "LoopBacks", "I"})); // - The output "C" of the loop graph then goes to input "X" of Add 'y' EXPECT_TRUE(root_.addEdge({"Iterator", "C", "y", "X"})); } protected: // We need a CoreData and Dissolve definition to properly instantiate DissolveGraph at present. - DissolveGraph root_; + TestGraph root_; NumberNode *i_{nullptr}; AddNode *x_{nullptr}, *y_{nullptr}; - IterableGraph *loop_{nullptr}; + IterableGraph *loopGraph_{nullptr}; }; +TEST_F(IterableGraphTest, RoundTrip) +{ + createGraph(); + + // Serialised graph TOML + SerialisedValue graphTOML; + ASSERT_NO_THROW(root_.serialise("graph", graphTOML)); + + // Deserialise from the stored TOML + auto deserialisedGraph = std::make_unique(); + ASSERT_NO_THROW(deserialisedGraph->deserialise(graphTOML["graph"])); + + // Complete round trip - re-serialise the result and compare it to the original TOML + SerialisedValue compareTOML; + ASSERT_NO_THROW(deserialisedGraph->serialise("graph", compareTOML)); + ASSERT_NO_THROW(UnitTest::compareToml("", graphTOML, compareTOML)); +} + TEST_F(IterableGraphTest, BasicNonLoopingSeries) { auto root = std::make_unique(); @@ -184,7 +203,7 @@ TEST_F(IterableGraphTest, NoRun) EXPECT_TRUE(iA); iA->set(1); - auto nLoops = loop_->findOption("N"); + auto nLoops = loopGraph_->findOption("N"); EXPECT_TRUE(nLoops); nLoops->set(0); @@ -194,13 +213,13 @@ TEST_F(IterableGraphTest, NoRun) // Check node versioning EXPECT_EQ(i_->versionIndex(), 0); - EXPECT_EQ(loop_->proxyInputs().versionIndex(), NodeConstants::InvalidVersion); + EXPECT_EQ(loopGraph_->proxyInputs().versionIndex(), NodeConstants::InvalidVersion); EXPECT_EQ(x_->versionIndex(), NodeConstants::InvalidVersion); - EXPECT_EQ(loop_->proxyOutputs().versionIndex(), NodeConstants::InvalidVersion); + EXPECT_EQ(loopGraph_->proxyOutputs().versionIndex(), NodeConstants::InvalidVersion); EXPECT_EQ(y_->versionIndex(), 0); // Loopbacks node only runs on iteration i > 0 - EXPECT_EQ(loop_->loopBacks()->versionIndex(), NodeConstants::InvalidVersion); + EXPECT_EQ(loopGraph_->loopBacks()->versionIndex(), NodeConstants::InvalidVersion); } TEST_F(IterableGraphTest, NoFeedback) @@ -219,7 +238,7 @@ TEST_F(IterableGraphTest, NoFeedback) EXPECT_TRUE(iA); iA->set(1); - auto nLoops = loop_->findOption("N"); + auto nLoops = loopGraph_->findOption("N"); EXPECT_TRUE(nLoops); // Zero iterations: We expect 1 + (xB = 1) = 1 + 1 = 2 @@ -230,13 +249,13 @@ TEST_F(IterableGraphTest, NoFeedback) // Check node versioning EXPECT_EQ(i_->versionIndex(), 0); - EXPECT_EQ(loop_->proxyInputs().versionIndex(), 0); + EXPECT_EQ(loopGraph_->proxyInputs().versionIndex(), 0); EXPECT_EQ(x_->versionIndex(), 0); - EXPECT_EQ(loop_->proxyOutputs().versionIndex(), 0); + EXPECT_EQ(loopGraph_->proxyOutputs().versionIndex(), 0); EXPECT_EQ(y_->versionIndex(), 0); // Loopbacks node only runs on iteration i > 0 - EXPECT_EQ(loop_->loopBacks()->versionIndex(), NodeConstants::InvalidVersion); + EXPECT_EQ(loopGraph_->loopBacks()->versionIndex(), NodeConstants::InvalidVersion); } TEST_F(IterableGraphTest, SingleFeedback) @@ -255,7 +274,7 @@ TEST_F(IterableGraphTest, SingleFeedback) EXPECT_TRUE(iA); iA->set(1); - auto nLoops = loop_->findOption("N"); + auto nLoops = loopGraph_->findOption("N"); EXPECT_TRUE(nLoops); // One iteration: We expect (LB = 2) + (xB = 1) = 2 + 1 = 3 @@ -266,13 +285,13 @@ TEST_F(IterableGraphTest, SingleFeedback) // Check node versioning EXPECT_EQ(i_->versionIndex(), 0); - EXPECT_EQ(loop_->proxyInputs().versionIndex(), 1); + EXPECT_EQ(loopGraph_->proxyInputs().versionIndex(), 1); EXPECT_EQ(x_->versionIndex(), 1); - EXPECT_EQ(loop_->proxyOutputs().versionIndex(), 1); + EXPECT_EQ(loopGraph_->proxyOutputs().versionIndex(), 1); EXPECT_EQ(y_->versionIndex(), 0); // Loopbacks node only runs on iteration i > 0 - EXPECT_EQ(loop_->loopBacks()->versionIndex(), 0); + EXPECT_EQ(loopGraph_->loopBacks()->versionIndex(), 0); } TEST_F(IterableGraphTest, ExtendedFeedback) @@ -291,7 +310,7 @@ TEST_F(IterableGraphTest, ExtendedFeedback) EXPECT_TRUE(iA); iA->set(1); - auto nLoops = loop_->findOption("N"); + auto nLoops = loopGraph_->findOption("N"); EXPECT_TRUE(nLoops); /* @@ -318,27 +337,27 @@ TEST_F(IterableGraphTest, ExtendedFeedback) // Check node versioning EXPECT_EQ(i_->versionIndex(), 0); - EXPECT_EQ(loop_->proxyInputs().versionIndex(), 9); + EXPECT_EQ(loopGraph_->proxyInputs().versionIndex(), 9); EXPECT_EQ(x_->versionIndex(), 9); - EXPECT_EQ(loop_->proxyOutputs().versionIndex(), 9); + EXPECT_EQ(loopGraph_->proxyOutputs().versionIndex(), 9); EXPECT_EQ(y_->versionIndex(), 0); // Loopbacks node only runs on iteration i > 1 - EXPECT_EQ(loop_->loopBacks()->versionIndex(), 8); + EXPECT_EQ(loopGraph_->loopBacks()->versionIndex(), 8); } TEST_F(IterableGraphTest, ReleaseLoopBack) { createGraph(); - const auto nEdges = loop_->edges().size(); + const auto nEdges = loopGraph_->edges().size(); - auto flagged = loop_->proxyInputs().findOutput("I"); + auto flagged = loopGraph_->proxyInputs().findOutput("I"); - loop_->removeEdge({"x", "Result", "LoopBacks", "I"}); + loopGraph_->removeEdge({"x", "Result", "LoopBacks", "I"}); - ASSERT_EQ(loop_->loopEdges().size(), 0); - ASSERT_EQ(loop_->edges().size(), nEdges); + ASSERT_EQ(loopGraph_->loopEdges().size(), 0); + ASSERT_EQ(loopGraph_->edges().size(), nEdges); } TEST_F(IterableGraphTest, UpstreamChange) @@ -357,7 +376,7 @@ TEST_F(IterableGraphTest, UpstreamChange) EXPECT_TRUE(iA); iA->set(1); - auto nLoops = loop_->findOption("N"); + auto nLoops = loopGraph_->findOption("N"); EXPECT_TRUE(nLoops); /* @@ -373,14 +392,14 @@ TEST_F(IterableGraphTest, UpstreamChange) // Check node versioning EXPECT_EQ(i_->versionIndex(), 0); - EXPECT_EQ(loop_->proxyInputs().versionIndex(), 99); + EXPECT_EQ(loopGraph_->proxyInputs().versionIndex(), 99); EXPECT_EQ(x_->versionIndex(), 99); - EXPECT_EQ(loop_->proxyOutputs().versionIndex(), 99); + EXPECT_EQ(loopGraph_->proxyOutputs().versionIndex(), 99); EXPECT_EQ(y_->versionIndex(), 0); // Loopbacks node only runs on iteration 0 < i <= nLoops // (in 100 runs, loop backs up version 99 times, starting from -1) - EXPECT_EQ(loop_->loopBacks()->versionIndex(), 98); + EXPECT_EQ(loopGraph_->loopBacks()->versionIndex(), 98); /* * Alter upstream number node and run for another 100 iterations @@ -395,14 +414,14 @@ TEST_F(IterableGraphTest, UpstreamChange) // Check node versioning EXPECT_EQ(i_->versionIndex(), 1); - EXPECT_EQ(loop_->proxyInputs().versionIndex(), 199); + EXPECT_EQ(loopGraph_->proxyInputs().versionIndex(), 199); EXPECT_EQ(x_->versionIndex(), 199); - EXPECT_EQ(loop_->proxyOutputs().versionIndex(), 199); + EXPECT_EQ(loopGraph_->proxyOutputs().versionIndex(), 199); EXPECT_EQ(y_->versionIndex(), 1); // Loopbacks node only runs on iteration 0 < i <= nLoops // (after another 100 runs, loop backs up version a further 99 times, starting from 98) - EXPECT_EQ(loop_->loopBacks()->versionIndex(), 197); + EXPECT_EQ(loopGraph_->loopBacks()->versionIndex(), 197); } } // namespace UnitTest diff --git a/tests/nodes/subGraph.cpp b/tests/nodes/subGraph.cpp index b1d6f29aaf..dd7ed49a18 100644 --- a/tests/nodes/subGraph.cpp +++ b/tests/nodes/subGraph.cpp @@ -4,6 +4,7 @@ #include "nodes/add.h" #include "nodes/dissolve.h" #include "nodes/number.h" +#include "tests/testing.h" #include namespace UnitTest @@ -71,6 +72,21 @@ class SubGraphTest : public ::testing::Test wB_ = w_->findInput("Y"); ASSERT_TRUE(wB_); wB_->set(Number{5}); + + // Create a mapped input on GraphA by creating an edge to it + EXPECT_TRUE(root_.addEdge({"x", "Result", "GraphA", "C"})); + + // Connect the mapped input on GraphA internally to it's "z" node + EXPECT_TRUE(graphA_->addEdge({"Inputs", "C", "z", "X"})); + + // Connect y result to z + EXPECT_TRUE(graphA_->addEdge({"y", "Result", "z", "Y"})); + + // Connect z result to graphA output, creating a mapped output + EXPECT_TRUE(graphA_->addEdge({"z", "Result", "Outputs", "D"})); + + // Connect GraphA mapped output "D" to node "w" + EXPECT_TRUE(root_.addEdge({"GraphA", "D", "w", "X"})); } protected: @@ -81,8 +97,45 @@ class SubGraphTest : public ::testing::Test std::shared_ptr xA_{nullptr}, xB_{nullptr}; std::shared_ptr yA_{nullptr}, yB_{nullptr}; std::shared_ptr wB_{nullptr}; + + // Basic sub-graph connection test + void connect() + { + // Create a mapped input on GraphA by creating an edge to it + EXPECT_TRUE(root_.addEdge({"x", "Result", "GraphA", "C"})); + + // Connect the mapped input on GraphA internally to it's "z" node + EXPECT_TRUE(graphA_->addEdge({"Inputs", "C", "z", "X"})); + + // Connect y result to z + EXPECT_TRUE(graphA_->addEdge({"y", "Result", "z", "Y"})); + + // Connect z result to graphA output, creating a mapped output + EXPECT_TRUE(graphA_->addEdge({"z", "Result", "Outputs", "D"})); + + // Connect GraphA mapped output "D" to node "w" + EXPECT_TRUE(root_.addEdge({"GraphA", "D", "w", "X"})); + } }; +TEST_F(SubGraphTest, RoundTrip) +{ + createGraph(); + + // Serialised graph TOML + SerialisedValue graphTOML; + ASSERT_NO_THROW(root_.serialise("graph", graphTOML)); + + // Deserialise from the stored TOML + auto deserialisedGraph = std::make_unique(); + ASSERT_NO_THROW(deserialisedGraph->deserialise(graphTOML["graph"])); + + // Complete round trip - re-serialise the result and compare it to the original TOML + SerialisedValue compareTOML; + ASSERT_NO_THROW(deserialisedGraph->serialise("graph", compareTOML)); + ASSERT_NO_THROW(UnitTest::compareToml("", graphTOML, compareTOML)); +} + TEST_F(SubGraphTest, Serialisation){ // createGraph(); // @@ -104,42 +157,12 @@ TEST_F(SubGraphTest, Serialisation){ TEST_F(SubGraphTest, Connections) { createGraph(); - - // Create a mapped input on GraphA by creating an edge to it - EXPECT_TRUE(root_.addEdge({"x", "Result", "GraphA", "C"})); - - // Connect the mapped input on GraphA internally to it's "z" node - EXPECT_TRUE(graphA_->addEdge({"Inputs", "C", "z", "X"})); - - // Connect y result to z - EXPECT_TRUE(graphA_->addEdge({"y", "Result", "z", "Y"})); - - // Connect z result to graphA output, creating a mapped output - EXPECT_TRUE(graphA_->addEdge({"z", "Result", "Outputs", "D"})); - - // Connect GraphA mapped output "D" to node "w" - EXPECT_TRUE(root_.addEdge({"GraphA", "D", "w", "X"})); } TEST_F(SubGraphTest, Flow) { createGraph(); - // Create a mapped input on GraphA by creating an edge to it - EXPECT_TRUE(root_.addEdge({"x", "Result", "GraphA", "C"})); - - // Connect the mapped input on GraphA internally to it's "z" node - EXPECT_TRUE(graphA_->addEdge({"Inputs", "C", "z", "X"})); - - // Connect y result to z - EXPECT_TRUE(graphA_->addEdge({"y", "Result", "z", "Y"})); - - // Connect z result to graphA output, creating a mapped output - EXPECT_TRUE(graphA_->addEdge({"z", "Result", "Outputs", "D"})); - - // Connect GraphA mapped output "D" to node "w" - EXPECT_TRUE(root_.addEdge({"GraphA", "D", "w", "X"})); - // Run w - all nodes should update EXPECT_EQ(w_->run(), NodeConstants::ProcessResult::Success); EXPECT_EQ(x_->versionIndex(), 0); diff --git a/tests/testing.cpp b/tests/testing.cpp index 810804bda4..aae5b6900b 100644 --- a/tests/testing.cpp +++ b/tests/testing.cpp @@ -427,9 +427,12 @@ void compareToml(std::string location, SerialisedValue toml, SerialisedValue tom if (toml.is_table()) { ASSERT_TRUE(toml2.is_table()) << location; + auto tab1 = toml.as_table(); + auto tab2 = toml2.as_table(); for (auto &[k, v] : toml.as_table()) { - ASSERT_TRUE(toml2.contains(k)) << location << "." << k << std::endl << "Expected:" << std::endl << toml[k]; + auto result = toml2.contains(k); + ASSERT_TRUE(result) << location << "." << k << std::endl << "Expected:" << std::endl << toml[k]; compareToml(std::format("{}.{}", location, k), v, toml2.at(k)); } }