Repository navigation
feat: add the LSH fast layout algorithm - #343
Merged
Merged
Conversation
smehringer
force-pushed
the
final_fast_layout
branch
from
September 24, 2026 09:00
60aad46 to
a220fa2
Compare
Member
|
Documentation preview available at https://docs.seqan.de/preview/seqan/chopper/343 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #343 +/- ##
==========================================
+ Coverage 93.35% 94.59% +1.23%
==========================================
Files 20 25 +5
Lines 753 1295 +542
Branches 18 21 +3
==========================================
+ Hits 703 1225 +522
- Misses 50 70 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
smehringer
force-pushed
the
final_fast_layout
branch
from
September 28, 2026 10:23
95dc15c to
fce830c
Compare
smehringer
force-pushed
the
final_fast_layout
branch
from
September 29, 2026 10:12
ffb3a48 to
7597f19
Compare
lsh_sim_approach runs concurrently for different merged bins, but concurrent_timer::start() and stop() write shared, non-atomic time points; only operator+=() is thread-safe. Besides wrong timings, stop()'s assertion stop_point >= start_point failed in Debug when another thread restarted the timer in between (12 of 40 runs of the new fast layout recursion test with 8 threads). Time with local serial_timers and add them to the configuration's timers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
No test reached fast_layout_recursion and add_level_to_layout: a merged bin needs at least 64 * tmax user bins for that, and the largest fast layout test had 96 user bins with tmax = 64. With tmax = 4 and 2000 user bins, each of the 4 top-level merged bins (about 500 user bins) is laid out recursively, and the DP layout handles the level below. The layout is checked structurally: every user bin is stored exactly once, merged and stored technical bins do not overlap, and there is one max bin entry per lower-level IBF. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
find_best_partition computed union_estimate - current_partition_size and asserted that the union estimate is not smaller. HyperLogLog estimates are not monotonic under merging: at the switch from linear counting to the raw estimate, a superset can be estimated smaller (4 of 800000 steps in a probe with 12 sketch bits, by up to 51). Debug builds then aborted and Release builds wrapped to a huge penalty that excluded the partition. Clamp the penalty to 0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The documentation claimed that the threshold is multiplied by max(1.01, sum / (threshold * max_bins)) and that the number of split user bins is clamped. The code sets the threshold to max(threshold + 1, sum / max_bins), and the number of split user bins cannot exceed the number of technical bins, which is only asserted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
post_process_clusters sorted the clusters after the first tmax by the cardinality of their largest user bin, with 0 for empty clusters. This put empty clusters last only if every non-empty cluster has a cardinality estimate above 0. Otherwise, a non-empty cluster could end up among the empty ones. Debug builds then failed the is_partitioned assertion, and in Release builds lsh_sim_approach stopped at the first empty cluster, so the user bins of such clusters were not assigned. Sort by (non-empty, cardinality) instead. For all other clusters, the order is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ined macros partition_user_bins_test.cpp and check_filenames.cpp use the CHOPPER_WORKAROUND_GCC_BOGUS_* macros without including chopper/workarounds.hpp. The undefined macros evaluate to 0 in #if, so the diagnostic pragmas were never active. This is why the GCC 16 -Warray-bounds workaround in partition_user_bins_test.cpp had no effect. -Wundef turns such mistakes into errors. For it, the feature test in fast_layout_cluster.hpp uses #ifdef, because __cpp_lib_containers_ranges is not defined before C++23 or with older standard libraries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fast_layout duplicated add_level_to_layout to record the top-level IBF. add_level_to_layout now sets the idx of the user bins it records and returns the maximum technical bin. fast_layout stores it as top_level_max_bin_id, fast_layout_recursion appends it to max_bins. The layouts are unchanged (compared on a recursive and a mixed input). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
find_best_partition always returned true and threw only if no partition was selected. With at least one partition, partition 0 is always selected unless its cost is SIZE_MAX, and lsh_sim_approach always passes at least one partition. Assert that instead of tracking best_p_found. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Document Cluster instead of the "foo" placeholder, including that is_valid accepts both valid and moved clusters. * Make the members private; nothing derives from Cluster. * Make the single-argument constructor explicit. * Replace the comments above LSH_find_representative_cluster and in very_similar_LSH_clustering, which described an older representation (cluster[i][0] == i). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
append_range and insert do the same for a std::vector<size_t>, but each toolchain only compiled and tested one of the two branches. Keep insert. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Document determine_split_bins. * Remove commented-out debug output and the unused max_id. * Remove the unused print_matrix.hpp include, include what is used instead of <numeric>, and fix the namespace in the file brief. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
general_layout started and stopped a local dp_algorithm_timer that was never read. Return the result of compute_layout directly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
remaining_clusters.insert(remaining_clusters.end(), x) is push_back(x). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The parameter holds the user bins per partition, and the caller passes partitions. Name it partitions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rename intital_partition_timer to initial_partition_timer. This also changes the column header in the --timing-output file to initial_partition_timer_in_seconds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
determine_best_number_of_technical_bins always uses the DP layout, so execute silently ignored fast_layout if determine_best_tmax was set. The command line already rejects the combination; the API now throws std::invalid_argument. The two execute_estimation_test cases with both options set expected the DP output. They are kept under #if 0 until the combination is supported, and same-named tests check the throw instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
It had a plain comment, so it was missing from the documentation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Many names were left over from the PHIBF, for which the algorithm was first written. Mostly, "partition" is replaced by "technical bin": * partition_user_bins -> lsh_distributed_ibf_layout, including the header, source and test file * find_best_partition -> find_best_technical_bin * partitions -> technical_bins for parameters and variables * initial_partition_timer -> top_level_lsh_distribution_timer and search_partition_algorithm_timer -> find_best_technical_bin_algorithm_timer The columns of these timers in the --timing-output file are renamed accordingly: initial_partition_timer_in_seconds -> top_level_lsh_distribution_timer_in_seconds and search_best_p_in_seconds -> find_best_technical_bin_algorithm_in_seconds.
Replace the test's own consistency check (check_layout) with seqan::hibf::layout::layout::validate. The test uses tmax 4, so the build would round each IBF up to 64 technical bins, and validate warns about the trailing empty bins (unexpected_empty_bins). The test accepts exactly these warnings and fails on any other finding. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
execute validates every layout with seqan::hibf::layout::layout::validate before it computes statistics or writes the layout file: the DP layout, the layout of determine_best_tmax and the fast layout. Errors throw std::invalid_argument; warnings, and notes in debug builds of the HIBF library, are printed to std::cerr. This replaces the debug-only check in fast_layout that all user bins are assigned. The verbose statistics are now printed after the validation, outside of the branch that computes the layout, so the condition excludes determine_best_tmax, which writes its own statistics. The tests that call execute check stderr: no findings, except for the note about the empty technical bins of few_ubs_fast_layout, which the fast layout produces by design. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
read_layout_file does not validate the layout; both subcommands of display_layout validate it right after reading. Document that read_layout_file does not validate. The layout of the display_layout tests has tmax 4, so each IBF ends with 60 empty technical bins instead of none (unexpected_empty_bins). The tests expect these warnings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
display_layout only caught parser errors. Any other exception, e.g., an invalid layout, terminated the program. Catch std::exception, print "[ERROR] <message>" and return EXIT_FAILURE, like chopper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
eseiler
force-pushed
the
final_fast_layout
branch
from
October 8, 2026 12:52
d9813a6 to
eac653a
Compare
eseiler
changed the title
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.
This would add the fast layout algorithm in its current status (dissertation status mehringer) to chopper.
Open questions: Should the fast layout algorithm instead be added to the hibf library?
Note to the code:
Currently most of the algorithm lives in
src/layout/fast_layout.cppDuring coding, I added some detail tests so there are two files with some extra code (that is again only used in fast_layout.cpp)
with their respective tests.