WIP: [FEATURE] Add the LSH fast layout algorithm. by smehringer · Pull Request #343 · seqan/chopper

smehringer · 2026-09-24T08:56:32Z

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.cpp
During coding, I added some detail tests so there are two files with some extra code (that is again only used in fast_layout.cpp)

determine_split_bins.hpp/cpp
fast_layout_cluster.hpp
with their respective tests.

seqan-actions · 2026-09-24T09:10:13Z

Documentation preview available at https://docs.seqan.de/preview/seqan/chopper/343

codecov · 2026-09-24T09:13:17Z

Codecov Report

❌ Patch coverage is 96.20253% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.50%. Comparing base (b2b2966) to head (3acabe6).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/layout/partition_user_bins.cpp	95.57%	12 Missing ⚠️
src/layout/fast_layout.cpp	93.57%	7 Missing ⚠️
include/chopper/layout/fast_layout_cluster.hpp	98.07%	1 Missing ⚠️
src/set_up_parser.cpp	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #343      +/-   ##
==========================================
+ Coverage   93.35%   94.50%   +1.14%     
==========================================
  Files          20       25       +5     
  Lines         753     1293     +540     
  Branches       18       22       +4     
==========================================
+ Hits          703     1222     +519     
- Misses         50       71      +21

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

0/0.0 is nan on gcc, but -nan on clang...

determine_best_number_of_technical_bins always uses the DP layout, so --fast-layout was silently ignored. Reject the combination right after parsing, before any sketching. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The parallel region had no num_threads clause, so it used all hardware threads (or OMP_NUM_THREADS) and ignored --threads. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

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>

post_process_clusters sorts the first tmax clusters by size, but lsh_sim_approach seeds only tmax - number_of_split_tbs partitions. The alternative, sorting the first number_of_remaining_tbs clusters, was measured on 60 synthetic inputs (400 to 50000 user bins). There was no systematic difference in expected HIBF size or query cost, so chopper keeps the current order. The directory contains the write-up, the measurement code (driver, patch for the alternative, build, run and analysis scripts) and the results. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

eseiler

Looks good. I pushed commits.

Overview

Fixes: everything before refactor: record the top level with add_level_to_layout. Each fix usually comes with a test that fails before the fix and passes after it.
Refactoring and docs: everything from refactor: record the top level with add_level_to_layout on.

`test: add the measurement of the post_process_clusters seeding variants`

This commit checks whether post_process_clusters should partially sort the first tmax clusters by size, or only the first tmax - number_of_split_bins.

With tmax, only tmax - number_of_split_bins of the sorted clusters are used as seeds. The remaining number_of_split_bins sorted clusters are then the first ones assigned to the merged bins. So the clusters assigned after seeding are not all sorted by cardinality: the first number_of_split_bins of them are still in the order of the partial sort.

It doesn't seem to make a difference (on rather small data), but I asked Claude to write it up. The commit can be removed after you have read it.

`determine_best_tmax` + `--fast-layout`

Using the two together throws, by design. The tests for the combination are deliberately kept under #if 0: we probably want to add this feature, and it should be pretty straightforward, but the PR is already big enough as it is :)

^{Edited by Claude for formatting and language (e.g., "latter former case")}

eseiler · 2026-09-29T22:17:48Z

+        // HyperLogLog estimates are not monotonic under merging: Where the estimate switches from linear counting to
+        // the raw estimate, the union can be estimated smaller than the partition. Clamp to 0 instead of underflowing.


Good to know :)

Doesn't happen often:

Probe: a superset was estimated smaller in 4 of 800,000 steps, by up to 51

eseiler · 2026-09-29T22:22:28Z

+    std::ranges::sort(clusters | std::views::drop(config.hibf_config.tmax),
+                      std::ranges::greater{},
+                      [&largest_user_bin_cardinality](Cluster const & c)
+                      {
+                          return std::tuple{!c.empty(), largest_user_bin_cardinality(c)};
+                      });


I am pretty sure that an empty cluster has a cardinality of 0 and any non-empty cluster also has a non-zero cardinality. So, the two properties should be equivalent. On the other hand, it also doesn't hurt to make it explicit.

yes, I think it is rather unnecessary but does not hurt so we can keep it

smehringer · 2026-09-30T07:18:51Z

+        // User bins that were not placed, either because at most tmax are placed or because the partitions ran out.
+        if (end < cluster.size())
+            remaining_clusters.emplace_back(cluster.begin() + end, cluster.end());



Interesting. How did you catch this?
(commit fix: do not drop user bins of large seed clusters)
I have a sanity check in fast_layout(..) which, in debug mode, checks that all user bins have been assigned to the partitioning. Did that check catch this case?

Yes, in Debug mode the assert fired

smehringer · 2026-09-30T07:44:17Z

+            path.push_back(tb);
+        }
+
+        ASSERT_GE(user_bin.number_of_technical_bins, 1u);


In the comment above it says, each TB stores EXACTLY 1 UB.
Wht does this test for greater or equal?

smehringer · 2026-09-30T07:49:38Z

+
+        ASSERT_GE(user_bin.number_of_technical_bins, 1u);
+        ASSERT_LE(user_bin.storage_TB_id + user_bin.number_of_technical_bins, max_technical_bins);
+        auto & ibf = get_ibf(path);


If I understand the code up until here correctly, the former for loop already initializes all possible paths representing ibfs.
The get_ibf function uses try_emplace so it never fails. But at this point we should directly access the map with path and if something fails, than the layout is not correctly set up, no?
So what I'm trying to say is we should use:

Suggested change

auto & ibf = get_ibf(path);

auto & ibf = ibfs.at(path);

smehringer · 2026-09-30T07:52:35Z

+
+    std::set<std::vector<size_t>> lower_level_ibfs{};
+    for (auto const & [path, bins] : ibfs)
+        if (!path.empty())


If there exists a path that is actually empty, doesn't that represent an empty technical bin? ANd that should not happen. This is not tested though

smehringer · 2026-09-30T07:54:01Z

+        EXPECT_TRUE(max_bin_ibfs.insert(max_bin.previous_TB_indices).second) << "duplicate max bin entry";
+    }
+
+    EXPECT_EQ(max_bin_ibfs, lower_level_ibfs);


UH, Google test can now compare std::set? nice.

smehringer · 2026-09-30T07:56:23Z

+    }
+
+    EXPECT_EQ(max_bin_ibfs, lower_level_ibfs);
+}


I find the whole test quite complex and potentially not correct (see other comments).
Is the Layout in text form too large to be directly checked?

smehringer · 2026-09-30T08:21:43Z

+
+Measurement behind the decision to keep how `post_process_clusters` (`src/layout/partition_user_bins.cpp`) orders the
+clusters for the fast layout.
+


DO we really want to keep this rather "extensive" benchmark which just checked if the sorting within post_process_clusters makes a difference?
I would rahter add a benchmark dp vs fast_layout. Or a forced recursive fast layout vs using the DP after the top level layout.

I know its work that has already been done but it seems rather insignificant to keep.

smehringer force-pushed the final_fast_layout branch from 60aad46 to a220fa2 Compare September 24, 2026 09:00

seqan-actions added lint and removed lint labels Sep 24, 2026

smehringer force-pushed the final_fast_layout branch from 95dc15c to fce830c Compare September 28, 2026 10:23

seqan-actions added lint and removed lint labels Sep 28, 2026

[FEATURE] Add the LSH fast layout algorithm.

7597f19

smehringer force-pushed the final_fast_layout branch from ffb3a48 to 7597f19 Compare September 29, 2026 10:12

smehringer requested a review from eseiler September 29, 2026 10:13

seqan-actions added lint and removed lint labels Sep 29, 2026

[HEADER TEST] Add <cassert>.

23b60fc

seqan-actions added lint and removed lint labels Sep 29, 2026

eseiler added 3 commits September 29, 2026 18:32

fix: missing include

0ee07d2

fix: partition test clang

73b705f

fix: just hardoce NaN string

a4370c3

0/0.0 is nan on gcc, but -nan on clang...

seqan-actions added lint and removed lint labels Sep 29, 2026

fix: lambda return deduction

1d93768

seqan-actions added lint and removed lint labels Sep 29, 2026

eseiler and others added 8 commits September 29, 2026 21:44

fix: use the configured number of threads in the fast layout

bb1aa72

The parallel region had no num_threads clause, so it used all hardware threads (or OMP_NUM_THREADS) and ignored --threads. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

seqan-actions added lint and removed lint labels Sep 29, 2026

eseiler and others added 15 commits September 29, 2026 23:04

refactor: use insert in Cluster::move_to unconditionally

821d265

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>

refactor: remove the unused timer in general_layout

bfd8fad

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>

refactor: use push_back for the remaining clusters

1828a12

remaining_clusters.insert(remaining_clusters.end(), x) is push_back(x). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

refactor: rename the positions parameter of find_best_partition

d216d8a

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>

fix: typo in the initial partition timer

7eeee80

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>

docs: document the members of Cluster

d7a03d6

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

docs: document LSH_find_representative_cluster with doxygen

44da2b7

It had a plain comment, so it was missing from the documentation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

docs: document the fast layout timers

27cc446

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

docs: document execute

e6fb149

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

seqan-actions added lint and removed lint labels Sep 29, 2026

eseiler approved these changes Sep 29, 2026

View reviewed changes

eseiler reviewed Sep 29, 2026

View reviewed changes

smehringer commented Sep 30, 2026

View reviewed changes

		// HyperLogLog estimates are not monotonic under merging: Where the estimate switches from linear counting to
		// the raw estimate, the union can be estimated smaller than the partition. Clamp to 0 instead of underflowing.


		Measurement behind the decision to keep how `post_process_clusters` (`src/layout/partition_user_bins.cpp`) orders the
		clusters for the fast layout.

Conversation

smehringer commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seqan-actions commented Sep 24, 2026

Uh oh!

codecov Bot commented Sep 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

eseiler left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Overview

test: add the measurement of the post_process_clusters seeding variants

determine_best_tmax + --fast-layout

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

smehringer commented Sep 24, 2026 •

edited

Loading

codecov Bot commented Sep 24, 2026 •

edited

Loading

eseiler left a comment •

edited

Loading

`test: add the measurement of the post_process_clusters seeding variants`

`determine_best_tmax` + `--fast-layout`