From 5e663d0bdc9e2dd225befcb293474083a20329a3 Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Wed, 22 Jul 2026 13:55:20 +0100 Subject: [PATCH 01/11] refactor and add toml round-trip test case --- tests/nodes/loop.cpp | 88 +++++++++++++++++++++++++------------------- 1 file changed, 51 insertions(+), 37 deletions(-) diff --git a/tests/nodes/loop.cpp b/tests/nodes/loop.cpp index 21ccf88364..9474228dfa 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,53 @@ 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"])); +} + TEST_F(IterableGraphTest, BasicNonLoopingSeries) { auto root = std::make_unique(); @@ -184,7 +198,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 +208,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 +233,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 +244,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 +269,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 +280,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 +305,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 +332,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 +371,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 +387,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 +409,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 From 91353255d82cdeb160b91a653ed6b45db4c8eb6d Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Wed, 22 Jul 2026 13:55:58 +0100 Subject: [PATCH 02/11] toml roundtrip test case --- tests/nodes/subGraph.cpp | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/nodes/subGraph.cpp b/tests/nodes/subGraph.cpp index b1d6f29aaf..1e6ee469a3 100644 --- a/tests/nodes/subGraph.cpp +++ b/tests/nodes/subGraph.cpp @@ -83,6 +83,34 @@ class SubGraphTest : public ::testing::Test std::shared_ptr wB_{nullptr}; }; +TEST_F(SubGraphTest, RoundTrip) +{ + 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"})); + + // 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"])); +} + TEST_F(SubGraphTest, Serialisation){ // createGraph(); // From 09e74d5c99b4734db03dde27d1cbe64e51dd54a9 Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Wed, 22 Jul 2026 13:58:42 +0100 Subject: [PATCH 03/11] still need to fix loop edges/ loop backs --- src/nodes/edge.cpp | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/src/nodes/edge.cpp b/src/nodes/edge.cpp index 616797bac3..07aa99e4df 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" @@ -38,11 +39,28 @@ 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 targetNode = parent->findNode(definition.targetNode); + 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 @@ -75,13 +93,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)) { From 6f6f588a28c343344775885ead4f83f1d938e3ef Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Wed, 22 Jul 2026 15:46:02 +0100 Subject: [PATCH 04/11] refactor loop graph - part 1 --- src/nodes/iterableGraph.cpp | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/nodes/iterableGraph.cpp b/src/nodes/iterableGraph.cpp index 6ae7583aa1..db01b37d2b 100644 --- a/src/nodes/iterableGraph.cpp +++ b/src/nodes/iterableGraph.cpp @@ -45,6 +45,8 @@ void IterableGraph::setLoopBacks() for (const auto &[name, param] : sources) loopBacks_->inputs().insert_or_assign(name, param); + + auto res = true; } // Release loopback by name @@ -128,23 +130,28 @@ 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; + // Refresh the graph loopbacks + setLoopBacks(); - loopEdges_.emplace_back(LoopEdge::makeLoopEdge(edge.release(), proxyInputs())); + // 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 nonInvertibleNode = dynamic_cast(parentGraph()->findNode(definition.sourceNode)) || + !loopBacks_->findInput(definition.targetInput); - addOutputLoopEdge(definition.sourceOutput, loopEdges_.back().get()); + // If not invertible, create and return a standard edge + if (nonInvertibleNode) + return Graph::addEdge(definition); - return true; - } + // Create loop edge + 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())); - return Graph::addEdge(definition); + return addOutputLoopEdge(definition.sourceOutput, loopEdges_.back().get()); } // Remove edge between nodes From a1ed808d032506478935bc46bc80c88f79cad62c Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Thu, 23 Jul 2026 10:32:15 +0100 Subject: [PATCH 05/11] finish sub graph round trip --- src/nodes/edge.cpp | 4 +-- tests/nodes/subGraph.cpp | 76 ++++++++++++++++------------------------ tests/testing.cpp | 5 ++- 3 files changed, 37 insertions(+), 48 deletions(-) diff --git a/src/nodes/edge.cpp b/src/nodes/edge.cpp index 07aa99e4df..45033e25c4 100644 --- a/src/nodes/edge.cpp +++ b/src/nodes/edge.cpp @@ -32,6 +32,8 @@ class EdgeConstructor : public Edge // Create an edge from the supplied definition std::unique_ptr Edge::create(Graph *parent, const EdgeDefinition &definition) { + auto targetNode = parent->findNode(definition.targetNode); + // Get source node and output auto sourceNode = parent->findNode(definition.sourceNode); if (!sourceNode) @@ -53,7 +55,6 @@ std::unique_ptr Edge::create(Graph *parent, const EdgeDefinition &definiti // The target node is the parent Graph's own Inputs node, so create a parameter link from the mapped input to the // targetInput - auto targetNode = parent->findNode(definition.targetNode); auto link = targetNode->findInput(definition.targetInput)->createParameterLink(definition.sourceOutput); if (!parent->addProxyInput(link.inputParameter, link.outputParameter)) { @@ -72,7 +73,6 @@ std::unique_ptr Edge::create(Graph *parent, const EdgeDefinition &definiti } // 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); diff --git a/tests/nodes/subGraph.cpp b/tests/nodes/subGraph.cpp index 1e6ee469a3..68492ed293 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 @@ -81,34 +82,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}; -}; -TEST_F(SubGraphTest, RoundTrip) -{ - createGraph(); + // 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"})); - // 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 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 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 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(); - // Connect GraphA mapped output "D" to node "w" - EXPECT_TRUE(root_.addEdge({"GraphA", "D", "w", "X"})); + connect(); // Serialised graph TOML - SerialisedValue graphTOML_; - ASSERT_NO_THROW(root_.serialise("graph", graphTOML_)); + 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"])); + 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){ @@ -133,40 +145,14 @@ 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"})); + connect(); } 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"})); + connect(); // Run w - all nodes should update EXPECT_EQ(w_->run(), NodeConstants::ProcessResult::Success); 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)); } } From 154bba822bc840b7593993188f5ea0ee3d0834aa Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Thu, 23 Jul 2026 12:37:25 +0100 Subject: [PATCH 06/11] serialise loop graphs ... but stil can't deserialise --- src/nodes/edge.cpp | 10 ++++++++ src/nodes/edge.h | 9 +++++++ src/nodes/iterableGraph.cpp | 47 ++++++++++++++++++++++++++++++++----- src/nodes/iterableGraph.h | 11 +++++++++ tests/nodes/loop.cpp | 11 ++++++--- 5 files changed, 79 insertions(+), 9 deletions(-) diff --git a/src/nodes/edge.cpp b/src/nodes/edge.cpp index 45033e25c4..4d54c63151 100644 --- a/src/nodes/edge.cpp +++ b/src/nodes/edge.cpp @@ -312,3 +312,13 @@ 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]["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 db01b37d2b..f94526d549 100644 --- a/src/nodes/iterableGraph.cpp +++ b/src/nodes/iterableGraph.cpp @@ -87,6 +87,14 @@ LoopEdge *IterableGraph::findLoopEdge(const EdgeDefinition &definition) const return {}; } +// Add edge between nodes +bool IterableGraph::addLoopEdge(std::unique_ptr edge, std::string_view source) +{ + loopEdges_.emplace_back(LoopEdge::makeLoopEdge(edge.release(), proxyInputs())); + + return addOutputLoopEdge(source, loopEdges_.back().get()); +} + // Add edge to node map Edge *IterableGraph::addOutputLoopEdge(std::string_view sourceOutput, Edge *edge) { @@ -136,11 +144,11 @@ bool IterableGraph::addEdge(const EdgeDefinition &definition) // 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 nonInvertibleNode = dynamic_cast(parentGraph()->findNode(definition.sourceNode)) || - !loopBacks_->findInput(definition.targetInput); + auto nonInvertible = dynamic_cast(parentGraph()->findNode(definition.sourceNode)) || + !loopBacks_->findInput(definition.targetInput); // If not invertible, create and return a standard edge - if (nonInvertibleNode) + if (nonInvertible) return Graph::addEdge(definition); // Create loop edge @@ -149,9 +157,7 @@ bool IterableGraph::addEdge(const EdgeDefinition &definition) if (!edge) return false; - loopEdges_.emplace_back(LoopEdge::makeLoopEdge(edge.release(), proxyInputs())); - - return addOutputLoopEdge(definition.sourceOutput, loopEdges_.back().get()); + return addLoopEdge(std::move(edge), definition.sourceOutput); } // Remove edge between nodes @@ -190,3 +196,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 9474228dfa..ca0102d0c4 100644 --- a/tests/nodes/loop.cpp +++ b/tests/nodes/loop.cpp @@ -77,12 +77,17 @@ TEST_F(IterableGraphTest, RoundTrip) createGraph(); // Serialised graph TOML - SerialisedValue graphTOML_; - ASSERT_NO_THROW(root_.serialise("graph", graphTOML_)); + 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"])); + 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) From c36f74cc3e2448508e3ec790d839b6babcc5e91e Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Thu, 23 Jul 2026 12:43:05 +0100 Subject: [PATCH 07/11] remove unused placeholder code --- src/nodes/iterableGraph.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/nodes/iterableGraph.cpp b/src/nodes/iterableGraph.cpp index f94526d549..55762fb111 100644 --- a/src/nodes/iterableGraph.cpp +++ b/src/nodes/iterableGraph.cpp @@ -45,8 +45,6 @@ void IterableGraph::setLoopBacks() for (const auto &[name, param] : sources) loopBacks_->inputs().insert_or_assign(name, param); - - auto res = true; } // Release loopback by name From e426c2d7758a28752d28dcda4093a607397fffc8 Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Thu, 23 Jul 2026 14:11:47 +0100 Subject: [PATCH 08/11] fully deserialise loop graphs --- src/nodes/edge.cpp | 1 + src/nodes/iterableGraph.cpp | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/nodes/edge.cpp b/src/nodes/edge.cpp index 4d54c63151..61fc8132a3 100644 --- a/src/nodes/edge.cpp +++ b/src/nodes/edge.cpp @@ -32,6 +32,7 @@ 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); // Get source node and output diff --git a/src/nodes/iterableGraph.cpp b/src/nodes/iterableGraph.cpp index 55762fb111..1f20a625e5 100644 --- a/src/nodes/iterableGraph.cpp +++ b/src/nodes/iterableGraph.cpp @@ -215,8 +215,8 @@ void IterableGraph::deserialise(const SerialisedValue &node) [this](const auto &value) { auto definition = toml::get(value); - auto edge = Edge::create( - this, {definition.sourceNode, definition.sourceOutput, definition.targetNode, definition.targetInput}); + auto edge = + Edge::create(this, {definition.sourceNode, definition.sourceOutput, "LoopBacks", definition.targetInput}); if (!edge) return false; From f80cecabb9b7f274ff5dc8abc68bcfd460635800 Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Thu, 23 Jul 2026 14:20:56 +0100 Subject: [PATCH 09/11] tidy --- src/nodes/iterableGraph.cpp | 4 +--- tests/nodes/subGraph.cpp | 21 +++++++++++++++------ 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/nodes/iterableGraph.cpp b/src/nodes/iterableGraph.cpp index 1f20a625e5..c569018fb3 100644 --- a/src/nodes/iterableGraph.cpp +++ b/src/nodes/iterableGraph.cpp @@ -88,9 +88,7 @@ LoopEdge *IterableGraph::findLoopEdge(const EdgeDefinition &definition) const // Add edge between nodes bool IterableGraph::addLoopEdge(std::unique_ptr edge, std::string_view source) { - loopEdges_.emplace_back(LoopEdge::makeLoopEdge(edge.release(), proxyInputs())); - - return addOutputLoopEdge(source, loopEdges_.back().get()); + return addOutputLoopEdge(source, loopEdges_.emplace_back(LoopEdge::makeLoopEdge(edge.release(), proxyInputs())).get()); } // Add edge to node map diff --git a/tests/nodes/subGraph.cpp b/tests/nodes/subGraph.cpp index 68492ed293..dd7ed49a18 100644 --- a/tests/nodes/subGraph.cpp +++ b/tests/nodes/subGraph.cpp @@ -72,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: @@ -107,8 +122,6 @@ TEST_F(SubGraphTest, RoundTrip) { createGraph(); - connect(); - // Serialised graph TOML SerialisedValue graphTOML; ASSERT_NO_THROW(root_.serialise("graph", graphTOML)); @@ -144,16 +157,12 @@ TEST_F(SubGraphTest, Serialisation){ TEST_F(SubGraphTest, Connections) { createGraph(); - - connect(); } TEST_F(SubGraphTest, Flow) { createGraph(); - connect(); - // Run w - all nodes should update EXPECT_EQ(w_->run(), NodeConstants::ProcessResult::Success); EXPECT_EQ(x_->versionIndex(), 0); From 2baadb690b312b95896d759a83f1a40a9e3c496d Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Thu, 23 Jul 2026 14:51:07 +0100 Subject: [PATCH 10/11] refactor --- src/nodes/edge.cpp | 1 + src/nodes/iterableGraph.cpp | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/nodes/edge.cpp b/src/nodes/edge.cpp index 61fc8132a3..23ad0b3c2b 100644 --- a/src/nodes/edge.cpp +++ b/src/nodes/edge.cpp @@ -318,6 +318,7 @@ void Edge::deserialise(const SerialisedValue &node) void LoopEdge::serialise(std::string tag, SerialisedValue &target) const { definition().serialise(tag, target); + target[tag]["targetNode"] = "LoopBacks"; target[tag]["analogue"] = analogue_; } diff --git a/src/nodes/iterableGraph.cpp b/src/nodes/iterableGraph.cpp index c569018fb3..d9e9838f3f 100644 --- a/src/nodes/iterableGraph.cpp +++ b/src/nodes/iterableGraph.cpp @@ -213,8 +213,8 @@ void IterableGraph::deserialise(const SerialisedValue &node) [this](const auto &value) { auto definition = toml::get(value); - auto edge = - Edge::create(this, {definition.sourceNode, definition.sourceOutput, "LoopBacks", definition.targetInput}); + auto edge = Edge::create( + this, {definition.sourceNode, definition.sourceOutput, definition.targetNode, definition.targetInput}); if (!edge) return false; From 556bbba08e30e93fd3ee98b47e24109f8e9a9881 Mon Sep 17 00:00:00 2001 From: RobBuchanan Date: Mon, 27 Jul 2026 13:12:52 +0100 Subject: [PATCH 11/11] bring target node check to front of Edge::create --- src/nodes/edge.cpp | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/src/nodes/edge.cpp b/src/nodes/edge.cpp index 23ad0b3c2b..29af27ee86 100644 --- a/src/nodes/edge.cpp +++ b/src/nodes/edge.cpp @@ -34,6 +34,18 @@ std::unique_ptr Edge::create(Graph *parent, const EdgeDefinition &definiti { // 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); @@ -73,20 +85,6 @@ std::unique_ptr Edge::create(Graph *parent, const EdgeDefinition &definiti return {}; } - // Get target node and input - 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};