Skip empty #2670
Skip empty #2670
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2670 +/- ##
=======================================
Coverage 93.91% 93.91%
=======================================
Files 27 28 +1
Lines 27657 27697 +40
Branches 1270 1275 +5
=======================================
+ Hits 25973 26013 +40
Misses 1647 1647
Partials 37 37
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -3822,6 +3853,7 @@ def __init__(self, tree): | |||
| self.tree = tree | |||
| self.more_trees = True | |||
| self.forward = True | |||
| self.no_skip_empty = True if tree.skip_empty is None else not tree.skip_empty | |||
There was a problem hiding this comment.
More natural to make skip_empty a parameter of the TreeIterator class, so we call it like:
return TreeIterator(tree, skip_empty=skip_empty)
| self.more_trees = self.more_trees and self.tree.prev() | ||
| if not self.more_trees: | ||
| raise StopIteration() | ||
| while True: |
There was a problem hiding this comment.
This looks correct, but I think we'd need a good bit of testing to check what happens when we have runs of empty trees in the middle. Although, come to think of it I don't think we can have two adjacent empty trees, so maybe this while look could be replace with something simpler?
For example, you could simply wrap this iterator like so:
trees = TreeIterator(tree)
if skip_empty:
for tree in trees:
if not tree.is_empty():
yield tree
else:
return trees
|
Closed due to #2600 (comment) |
Description
Based off #2668, and fixes #2600. I'm not sure if storing the
skip_emptyvalue as a python attribute in theTreeclass is the right approach (it's not what's done for root_threshold) but from the tests it appears as if the call signature ofTree()andts.trees()should be the same. Otherwise I would just have this as a parameter sent to theTreeIteratoriterator class from thets.trees()method, and not bother storing it in theTreeclass itself.Suggestions for other approaches welcome
PR Checklist: