No need to formulate flow for Terminal anymore #1357
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SouthEndMusic
approved these changes
Apr 8, 2024
There was a problem hiding this comment.
I had a look at why the tests don't fail as you mentioned. It is because the way the test models with terminal nodes in them are initialized (without any data other than the node location), the Terminal \ static table is never initialized and thus never written, and so the terminal object is empty which is why the problematic code was never run. This would still have thrown an error for models in which the Terminal \ static table does exist.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was missed from #1300. We don't store vertical flows anymore. Doing this was the only reason for
formulate_flows!for Terminal nodes.What I don't fully understand is why tests did not fail on this. Any test model with a Terminal should have failed since the 3-arg
add_flow!(graph, id, -q)it called was removed in #1300. This PR also removes the 4-arg version that is no longer used.