Improve batch-match coverage #998
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #998 +/- ##
==========================================
+ Coverage 93.16% 93.34% +0.18%
==========================================
Files 18 18
Lines 6462 6458 -4
Branches 1097 1095 -2
==========================================
+ Hits 6020 6028 +8
+ Misses 300 292 -8
+ Partials 142 138 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Ended up tweaking a few things here, for example ancestors are now packed into partitions using a greedy bin packing algorithm and the logic about how many partitions to use simplified. |
benjeffery
force-pushed
the
batch-coverage
branch
from
e5fa97f to
f0f3ef0
Compare
tsinfer/inference.py
Outdated
| if group_index == 0: | ||
| partitions.append(group_ancestors) | ||
| partitions = [ | ||
| group_ancestors, |
There was a problem hiding this comment.
stray comma causing profligate whitespace
tsinfer/inference.py
Outdated
| current_partition.append(ancestor) | ||
| current_partition_work += ancestor_lengths[ancestor] | ||
| partitions.append(current_partition) | ||
| parition_count = math.ceil(total_work / min_work_per_job) |
There was a problem hiding this comment.
typo "paritition" -> partition
There was a problem hiding this comment.
Fixed. I'm sprinkling these in now to prove that a free-range human wrote the code.
tsinfer/inference.py
Outdated
| sorted_ancestors = sorted( | ||
| group_ancestors, key=lambda x: ancestor_lengths[x], reverse=True | ||
| ) | ||
| partitions = [] |
There was a problem hiding this comment.
| partitions = [] | |
| partitions = [[] for _ in range(partition_count)] | |
| partition_lengths = [0 for _ in range(partition_count)] |
There was a problem hiding this comment.
Superseded by the heap code.
|
|
||
| # Use greedy bin packing - place each ancestor in the bin with | ||
| # lowest total length | ||
| for ancestor in sorted_ancestors: |
There was a problem hiding this comment.
How about we use a heapq for this?
heap = [(0, []) for _ range(partition_count]
for ancestor in sorted_ancestors:
sum_len, partition = heapq.heappop(heap)
partition.append(ancestor)
sum_len += ancestor_lengths[ancestor]
heapq.heappush(heap, (sum_len, partition))I think this does the same thing, but avoids the quadratic time complexity here.
There was a problem hiding this comment.
Very nice, I should have thought of this!
|
Fixed up in a063456 |
Fixes #972