Repository navigation
fix: report all levels and handle empty user bins in hibf_statistics - #349
Merged
Merged
Conversation
collect_bins copied each child IBF into its merged bin while iterating over the IBFs in max_bins order. Both layout algorithms sort max_bins by level, so a parent copied its child before the child's own merged bins were attached. Every IBF below level 1 was therefore reported as empty: its size and query cost were missing from the summary, and its split_tb_percentage was 0/0, which UBSan reports in Debug builds. Attach the child IBFs top-down instead, recursing into each attached child. This does not depend on the order of max_bins. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
User bins without k-mers made three statistics divide 0 by 0: * avg_factor of a level whose split bins are all empty, * l_tmax (expected_HIBF_query_cost) if all user bins are empty, * m_tmax if both the HIBF and the 64-HIBF it is compared to have a size of 0. They were printed as NaN or -nan, and UBSan reports them in Debug builds. If all user bins were empty, determine_best_tmax never accepted a tmax (NaN < best is false), and execute threw "The layout contains no user bins". Print "-" for such an avg_factor, as for levels without split bins. Use l_tmax = 0 (there are no k-mers to query) and m_tmax = 1 (both have the same size). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
No value printed with it can be NaN anymore: split_tb_percentage divides by the number of technical bins on the level (at least 1), avg_split_tb is only computed if the level has split bins, and avg_factor only if they contain k-mers. This also removes the trailing return type that was only needed for the "NaN" string literal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
|
Documentation preview available at https://docs.seqan.de/preview/seqan/chopper/349 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #349 +/- ##
==========================================
+ Coverage 94.60% 94.68% +0.07%
==========================================
Files 25 25
Lines 1297 1297
Branches 21 20 -1
==========================================
+ Hits 1227 1228 +1
+ Misses 70 69 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
eseiler
enabled auto-merge
October 12, 2026 12:56
eseiler
disabled auto-merge
October 12, 2026 13:00
This file contains hidden or 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
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.
I don't think all the cases where we could divide by zero could occur in the wild, but it's also not worth the time to disprove :)
Fixes two bugs in
hibf_statistics, the summary printed with--output-verbose-statisticsandwritten by
--determine-best-tmax.IBFs below level 1 were dropped.
collect_binscopied each child IBF into its merged bin whileiterating in
max_binsorder. Both layout algorithms sortmax_binsby level, so a parent copied itschild before the child's own merged bins were attached. Every IBF on level 2 or deeper was reported as
empty (0 TBs, 0 bytes,
NaNsplit percentage), and its size and query cost were missing fromsizeand
l_tmax. Child IBFs are now attached top-down, independent of the order ofmax_bins.hibf_statistics.three_levels, run withmax_binsin both orders.execute_test.chopper_layout_statistics_fast_layoutencoded the bug. Its expectation changes:level 2 goes from 0 to 64 TBs,
l_tmaxfrom 1.24 to 1.29, andsizefrom 672.6KiB to 685.3KiB.On
main, this test fails in Debug builds because UBSan reports the 0/0.Empty user bins caused 0/0. Three values were
NaN:avg_factorof a level whose split bins hold no k-mers,l_tmaxif all user bins are empty,m_tmaxif both the HIBF and the 64-HIBF it is compared to have size 0.With all user bins empty and
--disable-estimate-union,--determine-best-tmaxnever accepted atmax, and
executethrew "The layout contains no user bins". These three now print-,0.00and1.00.hibf_statistics.empty_user_bins.The
NaNspecial case into_string_with_precisioncan no longer be reached and is removed.Not in this PR: with union estimation enabled (the default), empty user bins make hibf divide by zero
(
sketch/toolbox.cpp:223), andexecutecan crash withstd::out_of_range.🤖 Generated with Claude Code