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
Additional_nodes DTWF/FIXED_PEDIGREE #2176
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2176 +/- ##
==========================================
+ Coverage 91.46% 98.68% +7.22%
==========================================
Files 20 11 -9
Lines 11210 3950 -7260
Branches 2280 900 -1380
==========================================
- Hits 10253 3898 -6355
+ Misses 521 28 -493
+ Partials 436 24 -412
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
algorithms.py
Outdated
@@ -771,6 +771,12 @@ def flush_edges(self): | |||
self.tables.edges.add_row(left, right, parent, child) | |||
self.edge_buffer = [] | |||
|
|||
def update_node_flag(self, node_id, flag): | |||
self.flush_edges() |
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.
Do we need this flush_edges? Should probably be done in the other function we just added?
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.
Adding flush_edges()
to store_additional_nodes_edges()
would flush the edges twice in some cases, as it is also called by store_node()
.
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.
I see - should we call flush_edges to the non-store-node path in store_additional_nodes_edges then?
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.
It's a surprising side-effect to have here in update_node_flag
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.
LGTM!
51b4381
to
4cf4cf3
Compare
algorithms.py
outline for trackingadditional_nodes
forDTWF
andFIXED_PEDIGREE
models.