-
Notifications
You must be signed in to change notification settings - Fork 19
feat: LoopGraph #2262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop2
Are you sure you want to change the base?
feat: LoopGraph #2262
Conversation
RobBuchananCompPhys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress! I think it would be worth investing some time while we're developing more tests to try and make test graph initialisation more efficient i.e. via some sort of TestGraph class hierarchy as commented above.
| * ---------------------------------- | ||
| */ | ||
| root_.x = 0; | ||
| root_.y = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something we need to do often when setting up test graph? It may be good to have a base class for these tests which takes ownership of root_ and zeroes the coordinates behind the scenes, so we don't have to think about it.
namespace UnitTest
{
class LoopGraphTest : public GraphTest // derives from public ::testing::Test
{
public:
LoopGraphTest() : GraphTest(), dissolve_(coreData_) {} // GraphTest could have default xtor args like centered = true //which give us a centered (x0, y0) graph
// Create a graph for testing
void createGraph()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suspicion would be that, in the longer term, most of these test graphs would be loaded from a TOML file instead of crafting the graphs by hand.
rprospero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a solid design to me and answers most of the questions that I had in the stand-up. I'm particularly please that we know that the loop won't have to be re-run if the user changes and downstream analysis nodes.
| Error | ||
| Error, | ||
| Debug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Watching this grow, I'm wondering if we might be better off with a third party logging library. We've already written half of one and we might get some free functionality from one that is pre-built
| for (auto n = 0; n < iterations_.asInteger(); ++n) | ||
| { | ||
| debug("{}LoopGraph({})::process() - iteration {}", GraphDebug::indent(), name(), n); | ||
|
|
||
| // Pull edges connected to our InputsNode *if* | ||
| if (n > 0) | ||
| { | ||
| for (auto &[inputName, edges] : proxyInputs_->inputEdges()) | ||
| { | ||
| for (const auto edge : edges) | ||
| { | ||
| debug("{}LoopGraph::process() - Pulling edge {}...\n", GraphDebug::indent(), edge->definition().asString()); | ||
| switch (edge->pull()) | ||
| { | ||
| case (NodeConstants::ProcessResult::Failed): | ||
| case (NodeConstants::ProcessResult::InputsNotSatisfied): | ||
| return NodeConstants::ProcessResult::Failed; | ||
| case (NodeConstants::ProcessResult::Success): | ||
| case (NodeConstants::ProcessResult::Unchanged): | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Process child nodes, returning early if we encounter an error | ||
| auto result = Graph::process(); | ||
| if (result == NodeConstants::ProcessResult::Failed || result == NodeConstants::ProcessResult::InputsNotSatisfied) | ||
| return NodeConstants::ProcessResult::Failed; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we had discussed your proposal during the stand-up, I had misunderstood and thought that you were putting this logic into the input nodes. Having this in the node answers 95% of my concerns.
| * ---------------------------------- | ||
| */ | ||
| root_.x = 0; | ||
| root_.y = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suspicion would be that, in the longer term, most of these test graphs would be loaded from a TOML file instead of crafting the graphs by hand.
| // first iteration of the loop. So, we only pull from inputs to our InputsNode (proxyInputs_) after the first | ||
| // iteration. Furthermore, we do this manually here rather than let InputsNode pull its own edges as otherwise we | ||
| // get into an infinite loop when the graph tries to run. In this sense the | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like there was more to come here!
RobBuchananCompPhys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My feeling is that there are some similarities we have converged on, but also differences in the complexity and robustness of the looping strategies. Personally, my preference would be to implement something simpler, which the other PR demonstrates (in about half the lines of code)! This economy, I find, is easier to reason about. However, there are short comings there which I think you have covered here particularly with regards to robustness of version indexing, and as you mentioned in the stand up, ensuring that the loop graph isn't re-traversed when an external node is changed, which I hadn't taken into account. So, ideally I'd like to go with the simpler option and see how it holds up for now, but if you and Adam think this is the one that's fine also.
fd93228 to
4452f29
Compare
This PR implements a
LoopGraphnode - a simpleGraph-type node which iterates over its children a number of times. It changes theInputsNodeto mirror its proxy output parameters to identically-named input parameters, allowing edges to be created to transfer data back to the beginning of the loop.The actual data flow in this PR is worth explicitly stating. In the unit test, when the node
y_is pulled to start the processing theLoopGraphis activated and processes it's input edges (only one, from the nodei_). It then proceeds to iterate its subgraph for the required number of cycles. Loopback edges are not pulled on the first iteration, only on every subsequent iteration in order to provide any new input data. This means that, unless the nodei_is modified, calling the graph again (as is performed in the unit test) will result in it beingUnchanged.The loopback of data is achieved in practice by essentially disconnecting the
InputsNodefrom natural node behaviour, principally overriding it'srun()function to not automatically pull input edges (which would cause an infinite loop) and letLoopGraphhandle this when necessary. Furthermore, propagation of "update required" flags is deliberately stopped at edges connecting to an input on anInputsNodeas again this would cause an infinite loop.Complicated, but hopefully a solid solution. Thoughts please!