Repository navigation
feat: validate layout - #339
Conversation
|
Documentation preview available at https://docs.seqan.de/preview/seqan/hibf/339 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #339 +/- ##
==========================================
- Coverage 99.79% 99.76% -0.03%
==========================================
Files 53 53
Lines 1934 2154 +220
Branches 7 8 +1
==========================================
+ Hits 1930 2149 +219
- Misses 4 5 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
918a233 to
a763396
Compare
|
…fraction validate_and_set_defaults sets empty_bin_fraction to 1 - subtracted / tmax, so that subtract_empty_bins(tmax, empty_bin_fraction) returns subtracted again. But tmax * empty_bin_fraction can exceed the integer tmax - subtracted by a rounding error, which std::ceil turned into one more empty bin. For example, tmax 192 and empty_bin_fraction 0.05 became 10/192 and then 11/192 on a second call. compute_layout then gave the Root-IBF one technical bin less than intended, and a config validated again (chopper writes it, the HIBF constructor validates it) changed its empty_bin_fraction. subtract_empty_bins now ignores an excess of up to tmax * 1e-12 over an integer. For tmax up to 65536 and fractions in steps of 0.001, 197151 of 1022976 combinations changed on a second call before, none do now. For a fraction given by the user, the result only changes where tmax * fraction is an integer, e.g., 56 instead of 57 empty bins for tmax 1600 and 0.035. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
update_header_node_data adds each lower-level IBF below its parent, so the entry of a parent IBF has to be processed before the entries of its children. It relied on the max bins being sorted by level and only checked this with an assertion in debug builds. If they are not sorted by level, sort a copy. compute_layout sorts the max bins by level, so the copy is only needed for other layouts, e.g., hand-written ones. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5616b40 to
ea824d0
Compare
smehringer
left a comment
smehringer
left a comment
There was a problem hiding this comment.
Have you checked a few real world layouts?
Here are some for RefSeq. Maybe a quick check if they pass.
DP RefSeq:
/srv/data/smehringer/evaluate_dp/refseq_new/tmax_changes/256.layout
Fast layout:
/srv/data/smehringer/fast_layout/refseq_new/lsh.layout
| size_t first{}; | ||
| size_t last{}; // inclusive | ||
| bool is_merged{}; | ||
| size_t user_bin{}; // 0 for merged bins |
There was a problem hiding this comment.
0 for merged bins might be unintuitive because there could be user bin 0. std::optional?
I feel like this struct doesn't help to understand the nature of how bins within the IBF are like (merged vs split representation).
There was a problem hiding this comment.
The struct is only used to represent the layout. You kinda need an alternative representation of the layout (that also behaves nicely in case the layout is wrong) such that you can do all the checks.
I let claude summarize it which is way easier to understand than what I came up with:
- path (
std::vector<size_t>): identifies an IBF by the merged bins leading to it from the Root-IBF. It's the same key asmax_bin::previous_TB_indicesand asuser_bin::previous_TB_indices.{}is the Root-IBF, and{2, 2}is the IBF below merged bin 2 of the IBF below merged bin 2 (printed as "IBF 2;2"). ibfs(std::map<path, ibf_bins>): one entry per IBF that occurs in the layout.ibf_bins: what one IBF contains, namely itsoccupied_bins(astd::vectorof them) plushas_max_bin. That flag is set when amax_binsentry matches the IBF, and drives the duplicate/missing max bin checks.occupied_bins: one technical-bin range[first, last]of that IBF. It is either a single or split user bin (user_bin= its index) or a merged bin (first == last,user_bin == bin_kind::merged). It's a run-length form of whatibf_bin_to_user_bin_idholds for that IBF in the built HIBF.
There was a problem hiding this comment.
we can get rid of ibf_bins if we are willing to use a separate set for the max bins, see last commit. both have pros and cons
| { | ||
| auto & bins = ibf.bins; | ||
| std::ranges::sort(bins); | ||
| bins.erase(std::ranges::unique(bins).begin(), bins.end()); // repeated merged bins; user bins are unique |
There was a problem hiding this comment.
Can you exmplain to me why we need to erase here?
There was a problem hiding this comment.
The user bins below a merged bin do not need to be consecutive. In such a case, the merged bin would be added multiple times (which would be an overlap).
There is also a test for that:
TEST(layout_test, validate)
{
layout_t const layout = valid_layout();
auto const [valid, diagnostics] = validate(layout);
EXPECT_TRUE(valid);
EXPECT_TRUE(diagnostics.empty());
// The user bins below a merged bin do not need to be consecutive.
// Order: UB 5, UB 0, UB 3 (IBF 2;2), UB 2, UB 1 (IBF 2), UB 4 (IBF 2;2).
layout_t interleaved = layout;
std::swap(interleaved.user_bins[2], interleaved.user_bins[4]);
EXPECT_TRUE(validate(interleaved).second.empty());
}| SPDX-FileCopyrightText: 2006-2026, Knut Reinert & Freie Universität Berlin | ||
| SPDX-FileCopyrightText: 2016-2026, Knut Reinert & MPI für molekulare Genetik | ||
| SPDX-License-Identifier: CC0-1.0 |
There was a problem hiding this comment.
REUSE wants a license for every file.
If you can't annotate it in the file itself, you can do a <filename>.license with the license info instead.
In general, it would probably make more sense to switch to a REUSE.toml at some point.
Such a file can be put in any directory and can set the license for multiple files, by, e.g., a regex.
This would, e.g., set the license for all files in the same directory (recursively):
# SPDX-FileCopyrightText: 2006-2026 Knut Reinert & Freie Universität Berlin
# SPDX-FileCopyrightText: 2016-2026 Knut Reinert & MPI für molekulare Genetik
# SPDX-License-Identifier: BSD-3-Clause
version = 1
[[annotations]]
path = "**"
precedence = "aggregate"
SPDX-FileCopyrightText = ["2006-2026, Knut Reinert & Freie Universität Berlin", "2016-2026, Knut Reinert & MPI für molekulare Genetik"]
SPDX-License-Identifier = "CC0-1.0"path = "**" could also be path = "**/*.out" and live in the test/snippet dir.
But that's a follow-up :D
Yes, Claude noticed that there are also chopper and raptor in my develop directory and decided by itself that it's a good idea to crossvalidate. Your two layouts also work: Layouts: I read them like raptor's
Both store tmax 256 and a version-1 config without |
smehringer
left a comment
smehringer
left a comment
There was a problem hiding this comment.
I like the last commit! When reading the code it is easier to just keep in mind some new structs but not wander what ibf_bins is. And the code does not seem more complex without it.
Otherwise just some nail polish.
| // Every user bin below a merged bin passes through it. Only the first one creates the IBF below the merged | ||
| // bin, and only then is the merged bin added. Hence, each merged bin occurs once. | ||
| auto const [below, created] = ibfs.try_emplace(path); | ||
| if (created) | ||
| ibf->second.push_back({technical_bin, technical_bin, bin_kind::merged}); | ||
| ibf = below; |
|
Not sure if I can merge or there is a todo from your side. If you read this, feel free to merge. |
Returns the number of levels of the HIBF that the layout describes: 0 without user bins, 1 if all user bins are in the top-level IBF, and one more for each level of merged bins. Only the user bins are considered. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
bool validate(config const & config, diagnostic_handler const & handler = throw_on_error) const checks whether the layout describes a consistent HIBF for the configuration. It reports each finding as a layout::diagnostic to the handler and returns whether the layout is valid. Checking stops at the first error. - Errors: e.g., missing, duplicate or out-of-range user bins, overlapping technical bins, missing, duplicate or invalid max bins, and technical bin counts that overflow during the build. - Warnings: an IBF that, with its empty bins, exceeds tmax, a lower-level IBF with a single bin, and an IBF that does not end with as many empty bins as empty_bin_fraction implies. - Notes: empty technical bins between the used technical bins of an IBF. The default handler, layout::throw_on_error, throws std::invalid_argument for an error, prints warnings to std::cerr, and prints notes only in debug builds of the library. A diagnostic can be formatted with std::format or printed with operator<<. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The constructor that takes a layout now calls layout.validate(configuration) after configuration.validate_and_set_defaults(). A layout that does not describe a consistent HIBF for the configuration throws std::invalid_argument instead of being built. Warnings are printed to std::cerr. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
81ccc19 to
eebb23e
Compare
Big, but thorough, which is probably a good thing considering there will be more work on layouts.