perf: four subsystems from the efficiency survey, each reviewed and each corrected - #117
Merged
Merged
Conversation
…M per column
`label`, `peptidoform` and `protein` were read with `TableFile::str`, which is
`out.push(a.value(k).to_string())` per row, and all three stayed resident for the whole
stage -- including the 16-22 minutes the sidecar spends training. Measured on
out_hye/psms_competed.parquet (879,027 rows): mean byte lengths 5.550 / 21.659 / 13.842,
which under mimalloc's 8-byte bins is 32 + 48 + 40 = 120 bytes and THREE live heap blocks
per PSM. 105 MB / 2.6M blocks at that table, 376 MB / 9.4M at the 3,133,636-row Astral
pool, 1.39 GB / 34.8M at the 11.6M rows the stage's own ceiling message names.
The block count is the half that matters: the engine dies at the kernel's per-process
mapping limit (1,048,576), not on memory.
Now:
- `label` is reduced to `Vec<bool> is_decoy` during the read (every read of it in this
stage is `== "decoy"`), so the column costs 1 byte per row and no per-row block;
- `peptidoform` and `protein` use `TableFile::str_flat` through a new `FlatStr` (offsets +
one text buffer): 29.7 and 21.8 bytes per row, two allocations each for the whole column.
120 -> about 51.5 bytes and 3n -> 5 live blocks per stage: 376 -> 162 MB at the Astral
pool, 1.39 -> 0.60 GB at 11.6M rows. I did NOT take the survey's ~34 bytes/row, which
assumes a dictionary for `protein`. The arithmetic behind it is right for the measured
8% cardinality, but a dictionary is 24 + 4 bytes/row when protein-group strings approach
one per row, which is worse than today; flat is better than `Vec<String>` at every
cardinality. The survey's claim that this "deletes n SipHash lookups" in the protein
interner is also not delivered: the interner still hashes once per row, now over a `&str`
into the flat buffer. `str_eq`/`str_flat` in mumdia-io had tests and zero callers; this
is the first caller of `str_flat`.
Equality: byte-identical psms_scored and byte-identical handoff. `StringArray::from(
Vec<String>)` IS `StringArray::from_iter_values` in arrow 59 (string_array.rs:96), so the
same values from a flat column build the same offsets+values buffers.
`the_flat_metadata_columns_write_the_same_scored_parquet` pins the scored table against
the previous `Vec<String>` construction over 300 rows; the existing handoff byte-identity
test now drives the flat columns.
The one behaviour that could have moved is the label validation, and it does not:
`fdr::validate_labels` names the offending VALUE and a bool cannot, so the scan runs on
the flat text during the read, remembers the first offending value, and bails with the
identical message at the identical point in the sequence of checks.
`an_unknown_label_is_still_refused_by_its_value` pins the old message ("TARGET" and the
empty string) and that nothing is written before it fires.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… ids `grouped_q` hashed group ids that the two lines above the call had just made dense, then read each winner back out of `HashMap::keys()` with four more probes per group. Two of the three calls have a group count close to the row count, so the largest hash table in the stage was being built to reproduce its own input: measured on out_hye/psms_competed.parquet, 746,772 distinct base_peptide_id and 879,018 distinct (peptidoform, charge) over 879,027 rows, against 69,958 distinct proteins. All three call sites already pass `&[u32]`, so the generic key is gone and the reduction indexes `Vec<Option<GroupBest>>`. `size_of::<Option<GroupBest>>()` is 24 (the bools give the Option its niche; the test asserts it, because that is the memory claim). At the 3,133,636-row Astral pool the precursor call goes from hashbrown's 4,194,304 buckets -- `best` 138 MB + `qmap` 71 MB + `ks` 12.5 MB live together at about 234 MB -- to 75 MB of dense slots plus the picked vectors, about 134 MB saved on each of the two large calls; at the 11.6M rows the stage's own ceiling message names, 839 MB against 372 MB. Time is NOT the reason and I am not claiming it: n + 4k probes into a 138 MB table is about 1.3 s per call, roughly 0.2% of a 19.4-min stage. I did not take the rider the survey suggested folding in here (`target_decoy_q(&[f64], &[bool])`, another 50 MB at that pool): it is in fdr.rs, outside this agent's file list. `base_peptide_id` is dense over the LIBRARY, not over the competed rows, so an unguarded array indexed by it is gigabytes at 203M precursors. `dense_group_ids` indexes directly only when `max + 1 <= max(4n, 1024)` and otherwise interns to 0..k, which costs the hash per row it always cost and bounds the array by the group count. Equality: byte-identical. `the_dense_group_reduction_reproduces_the_hashed_one` keeps the previous implementation verbatim in the test module and compares bit patterns against it, in both QModes, on both arms of the density gate, over four generated populations that contain exact score ties, NaN and NEG_INFINITY (the two scores the `or_insert` placeholder interacts with), decoys, entrapment rows and an all-skipped entrapment group. The key-iteration order the previous version depended on never reached the output -- every member of a tied block gets the same qmin and the totals are order-free -- but it was a live read of `HashMap` iteration order, which the project bans, and the dense walk is in ascending key order. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…PSM-row chunk bound, one full-window reference profile Four of the eight ranked findings for the features driver. Every one is byte-identical by construction; nothing in the feature arithmetic moves. 1. The confident-bounds pass (`bound_from_confident`, on by default) was a second full, single-threaded decode of the chromatogram table -- ~68 GB of traces at the HYE shape -- to learn two scalars from the ~0.84% of rows belonging to a confident candidate. docs/27 section 3.4 measured the stage at 7:15 with it against 5:05 without, so 30% of the stage on one thread. Row-group pruning cannot help on a production-shaped table, so the decode is split instead: `confident_subchunks` precomputes the (first_row, n_rows) list the sequential `while abs < end` loop already generated, and `confident_half_widths` decodes groups of them on up to BOUNDS_DECODERS = 8 threads, each with its own span, `ChromStream` and `NameTab`. Eight rather than every core because each decoder holds a parquet row group (~115 MB at the HYE shape); this keeps the pass under about a gigabyte, well below the chunk loop's peak. Equality: the sub-chunk grid is unchanged (still absolute, so a candidate straddling a boundary is split exactly where it was), the parts are concatenated in order, and per-sub-chunk `NameTab`s only renumber ids that `ChromChunk::rows` resolves through the table travelling with the chunk. The samples come out in the same ORDER, not merely the same multiset that `percentile` would need. `confident_half_widths_serial` keeps the old code under cfg(test) and `parallel_confident_bounds_match_the_serial_pass_sample_for_sample` compares the two sample vectors bit for bit at five chunk sizes and two span shapes. Measured (ignored bench `bench_confident_bounds_serial_against_parallel`, 60,000 production-shaped rows, 15 sub-chunks, 32-thread host): 195.6 ms serial against 39.1 ms, 5.00x. 2. `ChromStream::read_chunk_filtered` called `ListF32::append_row` twice per chromatogram row: `ListArray::value(k)` is `values.slice(..)`, one `Arc<dyn Array>` allocation per row per column, and the scratch buffer was a second full copy of the row's ~1,760 B payload before `push_row` copied it into the chunk. 77.6M allocations and ~68 GB of memcpy per pass, on the serial loader, on BOTH passes. `list_row` borrows the row out of the decoded batch instead, leaving the one copy the chunk keeps. Equality: `value_offsets()` is already adjusted for a sliced list and `PrimitiveArray::values()` for the child's offset, so the slice is exactly `value(k).values()`; a null row yields an empty slice, as the cleared scratch buffer did. `list_row_borrows_exactly_what_append_row_copied` compares the two on every row of four different slices of a list column with null, empty and varying-length rows. Measured (`bench_list_row_against_append_row`, extraction + copy loop only, not the parquet decode around it): 124.1 ms against 43.5 ms, 2.85x. 3. `plan_chunks` closed a chunk on chromatogram rows while the value buffers (`ValueMatrix`, `ext_vals`, `frag_feats`, `prelim`, `elu_lo`, `elu_hi`) are sized in PSM rows, and the ratio is data-dependent and unbounded: 15.3 at the docs/27 shape (212 MB of matrix), 9 at the engine default `top_n_fragments`, and with `retain_top_peaks = 5` about 5.2 GB in flight from the same unchanged constant. A chunk now closes on whichever of `chunk_rows` and `max_psm_rows` (CHUNK_PSM_ROWS = 2^16) binds first, bounding the matrix at ~203 MB whatever the shape. Equality: chunk boundaries move no value. `plan_chunks_bounds_psm_rows_as_ well_as_chromatogram_rows` pins the planner (and that it still never cuts a candidate), and `extended_features_are_chunk_invariant` now runs a third arm whose chunks are closed by the PSM limit and compares every f64 column bit for bit against the single-chunk arm. 4. The full-window reference profile was built in `coelution` and again, term for term identically, in `interference`. `Evidence::ref_profile_full` holds it, built once in `build_evidence` by `weighted_reference_full`. `chromatographic`'s third build is deliberately NOT folded in: it clamps the weights at zero and falls back to an unweighted sum, which differs whenever a predicted intensity is negative, and `index.rs` rejects only non-finite ones. ~6,600 mul-adds and two allocations per PSM, so ~1.7e10 operations and 5.2M allocations on a 2.6M-row run. Equality: `the_cached_full_window_reference_is_the_profile_both_families_ built` reimplements both old builds in the test and compares bit patterns on an Evidence that carries a negative predicted intensity, and asserts the clamped build differs there. Equality position: byte-identical output for all four. The strongest evidence is `extended_features_match_the_pre_permutation_build`, a golden digest over every extended feature value captured at another commit, which still holds; `ci/smoke.sh` prints SMOKE_OK with unchanged peptides.tsv and proteins.tsv hashes. Not done, with reasons: finding 3 (parallel main-pass decode) needs CHUNK_CHROM_ROWS divided by the decoder count to keep resident bytes flat and is unsized on the only benchmark that can separate its decode from its compute; finding 5 (whole-run string columns) and finding 8 (rt stored once per candidate) need a new `Col` variant in mumdia-io and an extract-side schema change respectively, both outside this subsystem; finding 6 (shared fragment_features/build_evidence preamble) is core-time only and its equality is conditional on `bound_features`, which the default sets but a config can unset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…coder fast paths Three changes in the table layer, each measured, none touching what any stage computes. 1. Dictionary encoding is off for the FLOAT and DOUBLE parquet leaves. parquet-rs enables a dictionary on every column and only falls back to PLAIN once it reaches 1 MB, i.e. 131,072 distinct f64 or 262,144 distinct f32. Every row-group cap here (65,536 and 131,072) is at or below that, so a near-unique float column could never fall back: it was written fully RLE_DICTIONARY, paying a dictionary page holding essentially every value plus a bit-packed index on top. The chromatogram list leaves were not exempt -- a shipped chromatograms.parquet has RLE_DICTIONARY and a dictionary page offset on both rt.list.item and intensity.list.item. Measured by rewriting the shipped artifacts (bench_rewrite_a_real_artifact, snappy both sides): features.parquet 172.6 -> 149.9 MB (-13.2%), write 1.6 -> 0.54 s; psms_competed 157.1 -> 136.4 MB (-13.2%), write 1.26 -> 0.43 s; chromatograms 216.8 -> 171.6 MB (-20.9%), write unchanged. A constant 327 bytes per row on the ~390-column tables (2,485 -> 2,158), so about 1.0 GB off the six-run HYE competed table and 0.5 GB off a 3.84 GB rescore handoff. The read side did not come out of the noise on this host and is not claimed. Only the float leaves, and that narrowing is measured, not assumed (bench_dictionary_on_the_non_float_column_shapes, 1M rows, dictionary on -> off): unique i32 4.57 -> 4.00 MB, run-length i32 2.32 -> 1.17 MB, three-valued u32 0.02 -> 0.19 MB (+1,168%), two-valued utf8 0.01 -> 0.45 MB (+4,763%). Disabling it for every non-string leaf, as first proposed, would multiply the small-enum integer columns by about twelve. It is not a universal win either: a float table small enough that its dictionary stays small still compresses better with one, which the smoke fixture's 284-row features.parquet shows (440 -> 450 KB, +2.4%); tables of that shape are kilobytes. 2. ListF32 resolves the offsets, the child's values and the validity bitmap once in `of` instead of per row. `ListArray::value(k)` returns an owned ArrayRef: an Arc allocation plus an atomic refcount bump on the shared values buffer, per row, discarded right after the copy. Measured 2.0x on the copy loop (7.1-7.4 ms against 14.3-14.8 ms for 200,000 rows), 36-37 ns per row. Two list columns are read twice per run, by features and then by quant: 108 M rows on the HYE benchmark, 2.15 billion on the immuno run. push_list_f32 and push_list_f32_flat go through the same view, so they lose the per-row allocation too. 3. push_bool, push_str_eq and push_opt_f64 gained the `null_count() == 0` fast path the numeric decoders already had. Noise on its own; it is a consistency fix in an otherwise careful decoder set. The codec() docstring claimed zstd was a wall-clock lever. Measured on chromatograms, float dictionaries off both sides: snappy 171.6 MB / write 6.7-7.0 s / read 3.9-4.3 s against zstd 70.4 MB (-59%) / write 6.7 s / read 4.3 s. It halves the largest artifact for no encode cost and the decode difference is inside the noise here. Corrected to say that. EQUALITY. Values-identical; the bytes of every artifact with a float column move, because the float leaves are a different parquet encoding, so content hashes change -- the same position the MUMDIA_PARQUET_COMPRESSION knob already documents. Every reader decodes the identical f32/f64; no float arithmetic is touched anywhere in the path. Changes 2 and 3 are byte-identical by construction. ci/smoke.sh prints SMOKE_OK and its peptides.tsv and proteins.tsv are byte-identical to the pre-change binary's (f0b5dc38.../5a65d304...), and all 92 parquet artifacts of the smoke run compare value-equal between the two binaries under pyarrow, for 0.9% fewer bytes in total. Tests: the new encodings are pinned leaf by leaf (floats lose the dictionary, candidate_id / charge / label keep it); the decoder fast paths are pinned against the OLD per-row loops including -0.0, NaN and the error row index; the list view is pinned against whatever `ListArray::value(k)` returned, for a null row, an empty row, a LargeList and a SLICED array whose offsets no longer start at zero. Four #[ignore]d benchmarks; each A/B builds both arms the same way outside the timer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…, stop materialising the scored strings Three of the four ranked quant findings. The fourth, and the larger half of the first, are blocked outside this file list (see below). 1. The accepted-candidate set is a bitset, not a `HashSet<u32>` (finding 1, the part that needs no new IO entry point). It is probed once per chromatogram row -- 14,306,517 times on the six-run HYE artifact -- to answer a question about a contiguous library row index. One bit per id over the range the accepted ids span is 1.4 MB at 10.9M precursors and 25 MB at 203M, and the probe becomes a shift and a load. The base comes from the set, not assumed to be 0, because a banded search offsets ids by `lib.global_offset`; an id range too wide to be worth a bit each falls back to hashing. Measured (`accepted_candidate_probe_arms`, i9-13900KS, release, each arm building its own set inside its own timer): 14.3M probes against 53,863 accepted of 10.9M, HashSet 160-170 ms, bitset 10.5 ms. The survey estimated 200-350 ms for the HashSet arm; 160-170 ms is what this machine measures, so the saving is ~150 ms per quant stage rather than ~200-340. 2. The chromatogram table is read row group by row group, in parallel, while both phases that consume the store already are. Row groups are disjoint, contiguous row spans, so each is read into its own `ChromStore` and the stores are concatenated in file order (`ChromStore::append`). Reads are chunked at `CHROM_ROWS_IN_FLIGHT` (1 << 19 rows), which bounds both the transient arrow buffers and the per-span stores waiting to be merged: about 235 MB each at the 55.5 list values per row measured on the HYE artifact. A table written before extract's `CHROM_ROW_GROUP_ROWS = 1 << 16`, whose groups hold 1,048,576 rows, therefore reads with a single reader instead of turning into a 14.9 GB regression. A row group whose `candidate_id` footer statistics fall outside the accepted range is never opened. On an unbanded run every group holds an accepted candidate so this does not fire; on a banded run it skips groups from the footer alone. Measured (`chromatogram_read_arms`, release, 32 threads, both arms reading the same file and building their own store): 120,000 rows of 8-point traces in 15 row groups, one pass 35.1-35.5 ms, row-group plan 12.8-19.5 ms over four runs. The survey could not confirm the 2.1x it inherited; this fixture gives 1.8-2.8x on a page-cached file with every candidate accepted, which is the decompression-bound part and is what scales to the 802 MB artifact. 3. The scored table's three string columns are read in the shape they are used: `peptidoform` and `protein_group` flat (`TableFile::str_flat`, one buffer plus offsets), `label` as the single bit every reader tests (`TableFile::str_eq`), and `label == "target"` only under consensus mode, where it is the only thing that reads it. `TableFile::str` on the three was 63 MB of `Vec<String>` spine plus ~42 MB of payload blocks -- 2.64 million live heap allocations held for the whole stage -- on the 879,027-row HYE table, and ~375 MB at pooled-Astral scale. `passes_quant_filter` now takes the decoy bit; a label that is neither spelling is still treated as a target, as `label == "decoy"` did. EQUALITY POSITION: byte-identical output, and checked rather than argued. * `ci/smoke.sh` end to end against a binary built from this branch and from `main`'s quant.rs on the same tree: the same `peptides.tsv` /`proteins.tsv` hashes (f0b5dc38.., 5a65d304..), and all 16 quant and LFQ parquet artifacts the smoke writes compare byte-for-byte identical (`cmp`, 16 identical, 0 differ). * The one thing that moves is internal and is tested: a candidate whose rows straddle a row-group seam dedups within each part only, so it holds two axis ids with identical values, and `peak_window` then takes the merged-sample union instead of the shared-axis accumulation. Those two paths are bit-identical for every `peak_window` case (the tree's own `peak_window_both` harness), and `splitting_a_candidates_axis_across_a_seam_does_not_move_its_peak_window` pins it directly for the seam case. Row-group pruning withdraws no check: the per-row rt/intensity length guard already runs only on kept rows. * New tests: the bitset against the `HashSet` it replaces (banded base, below the base, the hashed fallback); the flat/str_eq reads against the `Vec<String>` reads they replace, including an unrecognised label spelling; the span read against the single pass, asserting the seam really is exercised; pruning planned and its store equal to the full read; and `run()` end to end against two chromatogram artifacts differing only in row-group size, one with every row in its own group. BLOCKED, reported rather than reached for: * The larger half of finding 1 -- turning the accepted-candidate filter into a parquet `RowSelection` so `rt` and `intensity` are not decoded for the 94% of rows that are discarded -- needs a new entry point next to `TableFile::batches` in `crates/mumdia-io/src/table.rs`, which is outside this file list. The survey sizes it at 6.9-16.5 s -> 2.5-4 s per run. Without it, the parallel read above is the whole of what quant's load can save, and the memory bound in `CHROM_ROWS_IN_FLIGHT` is sized for full row-group buffers rather than the 5.6% that a selection would materialise. * Finding 4 (extract writing the same per-candidate RT grid once per fragment row) is in `stages/extract.rs`, outside this file list, and the survey already demotes it for quant's purposes. Not attempted: `status.to_string()` per output row. Avoiding it needs a `Col` variant that takes `&'static str`, which is in `mumdia-io`; the allocations are per output row (~54k), not per scored row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The survey's rank-4 finding proposed HANDOFF_BATCH_ROWS 250,000 -> 4,096, on the ground
that `flush_block` stages rows row-major and then gathers 387 columns back out at a
1,548-byte stride, so every element read costs a 64-byte line. The mechanism is real and
its own number reproduces; the prescribed change does not survive the rest of the write,
so nothing moves and the measurement is committed instead.
`handoff_block_size_end_to_end` (#[ignore]d, 262,144 rows x 387 f32, release, min of
three per arm, three whole runs):
end to end transpose only
250,000 5.3-5.5 s/Mrow 2.6-3.4 s/Mrow 387.0 MB staged
131,072 8.3-9.7 2.30 202.9
65,536 8.5-11.1 101.4
16,384 9.2-10.3 2.12 25.4
4,096 9.2-13.0 0.42 6.3
1,024 12.2-16.2 0.39 1.6
So the transpose in isolation IS 6-8x cheaper once the block is L2-resident, close to the
survey's 6.7x. It is not the binding cost: the parquet encoder's per-batch work at 387
columns grows faster than the transpose shrinks, and 4,096 is 1.7x SLOWER end to end.
Every arm pays for what it uses -- `with_block_rows` now re-reserves the staging buffer
too, so a benchmark arm at N allocates exactly what a build with HANDOFF_BATCH_ROWS = N
would, and the transpose-only section allocates its own stage and its own column vectors
inside its own timer.
The 387 MB of staging the default keeps is transient and, as the survey itself noted, not
at the measured peak: under `rescore.strict` with `nn_torch` there is no feature matrix at
handoff time and the engine sits near 1.4 GB while the 9.3 GB process-tree peak happens
later inside the worker. Trading 1.7x of the handoff write for memory that is not at the
peak is not a trade worth making, so the constant stays and now carries the numbers.
Equality: nothing in the shipped path changed. The one non-test edit is a doc comment;
`with_block_rows` and the benchmark are `#[cfg(test)]`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…le length in release, and correct three claims the review refuted An adversarial review of 3912260 raised eight issues. Five held and are fixed; three are corrected claims rather than code. Nothing in the feature arithmetic moves: 318 workspace tests pass, `extended_features_match_the_pre_permutation_build` and both chunk-invariance tests still hold, and `ci/smoke.sh` prints SMOKE_OK. FIXED 1. The memory bound was false off the default, and this was the review's strongest point. `BOUNDS_DECODERS = 8` was a PER-CALL cap whose doc comment justified itself with "keeps the pass under about a gigabyte", which assumed one `features::run` in flight. It is not: `run_groups.rs:559` calls it from inside `chunk.par_iter()` when `groups.parallel > 1`, `run_experiment.rs:763` from `.par_iter()` over `process_run` when `experiment.parallel_runs > 1`, and `rayon::current_num_threads()` reports the whole pool regardless of nesting. At `groups.parallel = 4` that was 32 concurrent row groups, ~3.7 GB by the commit's own ~115 MB/decoder figure, against the ~0.46 GB main opened. `DecoderLease` makes the budget process-wide. It is take-what-is-left rather than a blocking semaphore, because the lease is acquired inside a rayon worker and a worker that blocks cannot steal: a call that finds the budget empty gets zero and decodes on one thread, which is the serial pass. The bound is therefore `BOUNDS_DECODERS + (C - 1)` for C concurrent calls -- 11 rather than 32 at `groups.parallel = 4`, and exactly 8 at the default C = 1. `the_bounds_decoder_budget_is_process_wide` pins the arithmetic on a budget of its own, and `parallel_confident_bounds_match_the_serial_pass_sample_for_sample` gained a nested arm: four concurrent calls from inside a `par_iter`, contending for the budget, each still bit-identical to the serial pass. 2. The new cross-module invariant was enforced only in debug. `interference.rs` guarded `ref_profile_full.len() == tf` with `debug_assert_eq!`; in release a wrong length degraded silently (`sum_full_profile` 0, `peak_bounds` and the second-peak scan skipped, `pearson` over the shorter overlap). `Evidence` is `pub` with `pub` fields and this same change added three fixtures that set the field to `vec![]`. Both readers now check the length and rebuild through `weighted_reference_full` instead, which is free on the one real constructor. `a_wrong_length_full_window_reference_is_rebuilt_rather_than_read_short` runs empty, short and long profiles through both families and requires every value to match the correctly built Evidence. 3. Nondeterministic error selection. `collect::<Result<Vec<_>>>()` over a rayon iterator keeps whichever error a worker recorded first, so with two failing sub-chunk groups the reported message was arbitrary; the serial pass reported the first failure in table order. Now collected as `Vec<Result<_>>` and the first `Err` in group order is returned. 4. The parts are concatenated with `append` rather than `extend_from_slice` over owned vectors, which was a copy the change introduced while removing copies. 5. Test gaps. `ListF32::Large` was never executed -- the arm a polars-written table takes -- so `list_row_borrows_exactly_what_append_row_copied` now runs every slice through both offset widths, including the non-f32 error. `parallel_confident_bounds_...` now runs its five chunk sizes and two span shapes at four POOL SIZES (1, 3, 8, ambient), which is the only input that changes the sub-chunk partition; the independence was argued but never executed. The two old profile builds moved out of the test body into `#[cfg(test)]` transcriptions, and the profile test gained the shapes `build_evidence` cannot make: traces shorter and longer than the window, and a trace past the end of `pred` -- including the `0.0 * NaN` case where the old coelution build and the new one genuinely differ, with the `has_full` guard that makes it unreachable asserted alongside. CORRECTED, with the arithmetic 6. "~68 GB of memcpy per pass ... on BOTH passes" was about 2x too large, and the review is right. `read_chunk_filtered` tests `keep` and `continue`s BEFORE the two `list_row` calls, and did so before the change too. In the confident-bounds pass `keep` is the confident set, ~0.84% of rows, so that pass extracts ~0.33M rows (~0.65M extractions, ~0.6 GB), not 38.8M. The ~68 GB is the MAIN pass alone; both passes together are ~68.9 GB, not ~137 GB, and finding 2's saving is one pass's worth. The `list_row` doc comment now says so. 7. "byte-identical output" was wrong for the PSM-row chunk bound, and this is stronger than the review put it: not merely unproven but false. The review noted `ci/smoke.sh` runs one chunk and so cannot distinguish the planners. It cannot, but the question is now measured rather than left open. `moving_a_chunk_boundary_moves_parquet_bytes_above_one_row_group` writes a 300,000-row table through one `write_cols` call and through calls of 1,000, 40,000 and 68,523 rows -- 68,523 being the docs/27 PSM rows per chunk, i.e. exactly the boundary `CHUNK_PSM_ROWS` moves -- and all three differ from the single-call file. Below one row group (4,000 rows) they are identical, which is why the 61-row chunk-invariance fixture is byte-identical in all three arms and why that told us nothing. So: feature VALUES, row order and PIN bytes are invariant and tested; the features.parquet BYTES move when the cap binds, and the manifest's artifact hash moves with them. Nothing downstream reads them. 8. The benchmark's caveats are now written on it, and the stage-level projection is removed. `bench_confident_bounds_serial_against_parallel` reads a 105 MB fixture that is deliberately warmed, so both arms are CPU-bound decode of the page cache where production streams ~68 GB and can be storage-bound; its traces are `grid.clone()` plus a Lorentzian and so far more compressible than real ones; and 10% of its candidates are confident against ~0.84% in production. The two arms remain fair to each other, so the ratio stands as an UPPER BOUND, but the "takes the measured 2:10 of docs/27 section 3.4 to roughly a sixth" line is deleted from `BOUNDS_DECODERS`: docs/27 section 3.4 has not been re-measured. REJECTED Nothing outright. The review's remaining observation -- that `extended_features_are_chunk_invariant`'s third arm covers the VALUES under a binding PSM cap, so this is a claim-scope defect and not an identification risk -- is correct, and is why 7 is a corrected claim plus a test rather than a revert. The newly reachable mid-run chunk with `chrom_rows == 0` the review flagged as untested is handled (`read_chunk(0)` returns an empty chunk and those PSMs take the `rows.is_empty()` default path) and is left as it was: constructing it needs 65,536 consecutive PSM rows whose candidates have no chromatogram rows, which the extract contract does not produce, and a fixture for it would pin a shape that cannot occur. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An adversarial review of 42b8c25 raised eleven issues against the parallel chromatogram read, the accepted-candidate bitset and the flat scored-column reads. Seven hold and are fixed, two are corrections to the evidence rather than to the code, and two are rejected below with the arithmetic. One further defect of my own is fixed. THE EQUALITY BREAK IS REAL, AND IT WAS AN OUTPUT DIFFERENCE `ChromStore::append` left a candidate whose rows straddle a row-group seam holding two axis ids with identical values, and 42b8c25's defence was that `peak_window`'s shared-axis and merged-sample paths are bit-identical for every case. They are not. `rt_is_sorted` tests `rt[0] >= 0.0`, which `-0.0` satisfies, and `axis_strict` additionally needs only strict `<`, which `-0.0 < 1.0` satisfies, so an axis `[-0.0, 1.0, 2.0, 3.0]` is marked strict and the shared path walks it in VALUE order. The merge keys the union on `to_bits`, and `bits(-0.0) = 0x8000_0000` sorts after every positive f32, so the merged path walks `[1.0, 2.0, 3.0, -0.0]` with the profile permuted and `axis_sorted` false. Measured with the pre-fix `append` on that axis (one candidate, three fragments, cut after the first row), `(lo, hi, apex)`: apex hint one axis (shared) two axes (merged) none (-0.0, 1.0, -0.0) ( 3.0, -0.0, -0.0) 2.5 (-0.0, 2.0, 2.0) ( 1.0, 2.0, 2.0) 1.4 (-0.0, 1.0, 1.0) ( 1.0, 2.0, 1.0) `lo` and `hi` are the integration bounds, so those are different quantities for the same rows, selected by the writer's row-group size and -- through `rayon::current_num_threads()` in the read plan -- by `--threads`. Extract writes positive scan times, but `mumdia quant --chromatograms` takes a table written by anything, which is the standard the rest of this file already holds itself to (`axis_for`'s own bitwise comparison exists for exactly this value). Patching `peak_window`, or refusing the fast path for a leading `-0.0`, would move the SINGLE PASS's answer for such a table and so break equality with `main` in the other direction. Instead `append` now reproduces the single pass exactly, axis ids included: the leading rows of the appended part that continue the open candidate are deduped against the open window, through the same search `axis_for` uses (extracted as `ChromStore::open_axis_matching`, so there is one implementation), and the open candidate is carried across the seam instead of being closed. A span that kept no row no longer closes it either. There is then no case in which the read plan can reach the output, and `peak_window` needs no equality argument at all. FIXED * Seam dedup in `append`, above. `the_row_group_plan_builds_the_single_passes_store_exactly` compares every field of the store -- rows, axis table, AXIS IDS, name table, `rt_sorted`, open window -- over a 9-row fixture (two grids, a repeated candidate, an empty trace) cut at every single position and every pair of positions, plus an empty span and a `push` after an append. `a_seam_inside_an_axis_beginning_with_negative_zero_does_not_move_the_window` pins the case above. Both fail on the pre-fix `append` (verified by disabling the dedup), as does `reading_row_group_by_row_group_reproduces_the_single_pass`, which now compares stores rather than rows. * Thread dependence. The store is now identical whichever plan runs, so `--threads` cannot reach the output; `chrom_spans` takes the pool width as a parameter instead of reading it, so the plan is assertable on any host. * `a_row_group_outside_the_accepted_ids_is_never_opened` no longer fails on a 1-vCPU runner. It asserted `chrom_spans(...).expect(...)` under `keep_all`, where `pruned == 0` and `readers = (524288/4).clamp(1, threads)` collapses to 1, which returns `None`. It now passes 4 explicitly and asserts the 1-thread plan declines. `a_candidate_split_across_row_groups_quantifies_identically` was silently VACUOUS on such a host for the same reason; its fixture now carries 8 candidates against the scored table's 6, so groups are pruned and the span plan is taken at any thread count, and it compares the written parquet FILES byte for byte rather than three decoded columns. * The all-pruned fallback read the whole file. Its stated reason was that "the single pass still validates the projection and the row shape"; the row-shape half is wrong, because the rt/intensity length guard runs only on KEPT rows and nothing is kept. It now reads ONE row group, which checks the projection and the column types just as well, instead of 802 MB to produce an empty store in exactly the case pruning exists for. * The memory bound was denominated in rows and so bounded no memory. `CHROM_ROWS_IN_FLIGHT` is 1 << 19 rows, which is 235 MB at the 444 B/row of the HYE artifact and 8.4 GB at the 2,000-point traces a wide or unbounded `w_rt` produces. `RowGroupMetaData::total_byte_size` is not exposed by `row_group_stats` (mumdia-io, outside this file list), so readers are now also capped by `CHROM_BYTES_IN_FLIGHT` (256 MB) against the file's own compressed bytes per row times an assumed 6x expansion: HYE is 802 MB / 14,306,517 rows = 56 B/row, x6 x 65,536 = 22 MB per group, 256/22 = 11, so the row bound's 8 readers still wins and the shipped shape is unchanged; 2,000-point traces give 1.06 GB per group and fall to one reader. One reader is still one row group, as the single pass it falls back to also is, and that is documented rather than claimed away. * The net memory direction is up, not down, and is now stated in the code: the single reader this replaces held one 65,536-row group (~29 MB), so the parallel read costs ~204 MB of transient buffers against the 60-105 MB of `String` spine the flat scored reads give back -- about +100-145 MB on a stage that already holds a ~355 MB store, bought for a 3-4x faster load. * The parallel-read benchmark ran a regime the engine never enters: 8,192-row groups plan 64 readers, so all 15 spans landed in one `spans.chunks(readers)` chunk with no merge barrier. It now uses extract's 65,536-row groups, 16 of them (8 readers, 2 chunks), runs the arms in both orders because the first read warms the page cache, and adds an arm that keeps the 5.6% of rows a real run keeps. Measured (i9-13900KS, release, 32 threads, 1,048,576 rows of 8-point traces): every row kept, one pass 254-313 ms against 81-83 ms for the plan (3.1-3.8x); 5.6% kept, 105-112 ms against 25-31 ms (3.4-4.4x). The seam dedup is inside those numbers. * `CidSet::from_ids` computed `(top - base) as usize + 1`, which on a 32-bit `usize` wraps to 0 for the full u32 range and then indexes a zero-length bitset out of bounds. Computed in u64 now. (Mine, not the review's.) EVIDENCE CORRECTED * The review is right that 42b8c25's byte-identical smoke proved nothing about the changed path: the smoke's chromatogram artifact is 2,556 rows in ONE row group, so `chrom_spans` returned `None` at `stats.len() < 2` and the run executed `load_chrom_span`, a verbatim move of main's loop. Coverage now: `ci/smoke.sh` is SMOKE_OK with the same `peptides.tsv` / `proteins.tsv` hashes 42b8c25 recorded against main (f0b5dc38.., 5a65d304..), and, separately, that artifact was rewritten with pyarrow at 1, 8 and 64 rows per row group (2,556, 320 and 40 groups; same schema, snappy) and `mumdia quant --out-fragment --out-peak-bounds` run against each. All four outputs -- peptide, protein, fragment and peak-bounds parquet -- are byte-identical to the single-group run, at the default thread count and at `--threads 1`, 20 comparisons, 20 identical. The plans those runs took are not assumed: 18 of the 2,556 groups and 2 of the 320 are pruned, so the 8-row and 1-row layouts take the span path even at `--threads 1`, and `--out-peak-bounds` exercises the `keep_all` path that keeps every row. * Scope: `git show --stat 42b8c25` lists one file, and the task's file list names two. `quant_lfq.rs` is 303 lines of MaxLFQ arithmetic with no `TableFile`, no label column and no candidate set; nothing in this change reaches it, and it is untouched here too. REJECTED * "The `label` column is decoded twice under consensus mode." It is, and it stays. `label` carries two distinct values over all 879,027 rows of the HYE scored table, so it is dictionary encoded and the second `str_eq` pass costs one more read of the cheapest column in the artifact plus `nrows` bools, which is 879 KB, against the ~21 MB of `String` spine (879,027 x 24 B) plus payload blocks that one `str` read would have held for the WHOLE stage. Neither bit can be derived from the other -- a spelling that is neither is a target to the filter and not a consensus anchor -- and the second read is taken only under `peak_window_mode = consensus`, which is not the default. Documented at the read site as a cost rather than removed. * "The accepted-id collection lost its deduplication." Harmless at two orders of magnitude. `psms_scored` carries one row per candidate, so the transient `Vec` is the accepted ROW count: 53,863 ids, 215 KB, on the six-run HYE table, and 3.5 MB in the impossible case where all 879,027 rows pass. The three walks (min, max, fill) are over that, not over the 14.3M-row table the set is then probed against, and a repeated id would only set the same bit twice. Documented in `from_ids` and at the call site. KNOWN AND NOT CLOSED A row group whose `candidate_id` statistics omit a NULL could be pruned while the single pass would have kept that row. Closing it needs a null count from `row_group_stats`, which is mumdia-io's. It is documented on `chrom_spans` with the reason it is not a live risk: extract writes the column from a `Vec<u32>`, which has no null, and the single pass does not read a null as a candidate either -- it takes `a_cid.value(k)`, the raw slot behind the validity bitmap, whose content the decoder does not define. VALIDATION (from rust/mumdia) cargo fmt --check; cargo clippy --workspace --all-targets -- -D warnings; cargo test --workspace (318 + 80 tests, 0 failed); cargo build --release, binary copied out of the shared target directory, and `MUMDIA_BIN=<copy> bash ci/smoke.sh <tmp>` printing SMOKE_OK. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…em corrected An adversarial review of `perf/r2-rescore-stage` (147bc4d, ec35509, f68a166) raised nine issues against the flat/bit metadata columns, the indexed group reduction and the handoff block-size benchmark. All nine hold in substance. Three of them are quantitatively wrong in the reviewer's favour or against it, and one of them turns out to be worse than the review says. Nothing was rejected outright. WHAT THE REVIEW FOUND, AND WHAT CHANGED 1. `picked` was unbudgeted. `best.into_iter().flatten().collect()` cannot know how many `Some` there are -- `Option<T>` is not a `ConstSizeIntoIterator`, only arrays are -- so `collect` grows from nothing by doubling. Verified with rustc -O: len 3,133,636 lands on capacity 4,194,304. Now `Vec::with_capacity(n_groups)` (counted in the reduction loop) plus `extend`, which measures capacity == len exactly. The commit message's "about 134 MB saved on each of the two large calls" was wrong, and so is the reviewer's "~50 MB". Peak against peak at the Astral precursor call (k = n = span = 3,133,636, slot 24 B, hashbrown 4,194,304 buckets x 33 B): old, at `target_decoy_q`: best 138.4 + ks 12.5 + sd 50.1 + ranked 50.1 + q 25.1 = 276.2 new before this fix, at the collect: best 75.2 + picked 100.7 + its 50.3 predecessor = 226.2 new after this fix: collect 150.4, kernel 75.2 + 50.1 + 50.1 + 25.1 = 200.5 so 75.7 MB per call, not 134 and not 50. At 11.6M rows the commit's "839 against 372" is wrong at both ends: old 1,064 MB, unfixed new 882, fixed 742. 2. The density gate admitted a memory regression on the key it was written to guard. `span <= 4n` at 24 bytes per SLOT is 96 bytes per ROW -- 301 MB at the Astral pool -- against 138 MB for the `HashMap<u32, GroupBest>` it replaced and about 76 MB for its own interning arm. The crossover against the hash is span = 1.84n, and `base_peptide_id` against a 6-12M base-peptide library lands in (1.84n, 4n] exactly. The gate is `span <= n` now: `protein_id` and `precursor_id` (span == group count) still index directly, and the direct array can no longer exceed 24n, which is inside the interning arm's own bound. 3. The 162 MB / 0.60 GB resident figures ignored the text buffer's capacity. `str_flat` builds `data` from `String::new()` and appends one row at a time, so it ends in [len, 2 x len). The reviewer's 2x is the bound, not the measurement, and was applied inconsistently (2x to `peptidoform`, 1.55x to `protein`): measured with rustc -O, 879,027 rows x 21 B gives 1.19x and 3,133,636 gives 1.34x. Rather than restate the figure, `bytes()` now reports capacity (so `memlog` sees it) and `shrink` returns the slack once the concatenation is complete, which makes the documented per-row cost a resident cost. `gather` sizes its buffer from the kept rows instead of doubling. Also corrected: 1 + 29.7 + 21.8 = 52.5 bytes, not 51.5, and 5 live blocks, not the 4 the in-code comment claimed (the bits, plus offsets and text for each flat column). 4. "9.4M live mapping-backed allocations" is withdrawn. mimalloc serves 32-48 byte blocks out of segment pages; the only mapping-limit death this repository has measured (`stages/extract.rs`: 129,393 mappings at 180 GB, dying at ~290 GB) was on 240 KB-2.4 MB blocks, which mimalloc does map individually. The block count (3n -> 5) is kept, the `vm.max_map_count` framing is gone. 5. The benchmark's arms did not pay what the commit said. `with_block_rows` runs AFTER `HandoffWriter::new` has reserved 250,000 x nf x 4 = 387 MB, and both are inside the timer, so every arm allocated and freed 387 MB it would never allocate in a build at that size -- and the 250,000 arm paid it twice, which biases FOR the small arms, not against them. `HandoffWriter::with_block_size` now names the block size at construction, `new` delegates to it at `HANDOFF_BATCH_ROWS`, the benchmark uses it, and `with_block_rows` is back to changing only the flush boundary, with the false sentence removed. 6. "387.0 MB staged" was half the transient. `flush_block` holds all 387 gathered `Vec<f32>` alive until `RecordBatch::try_new`, which the file's own comment above the streaming handoff already said. The doc and the benchmark's own column now say ~774 MB. 7. The two texts disagreed about what was measured, and re-measuring makes it worse. With the arms fixed, on this machine, minima over three whole runs of min-of-three (s/Mrow): 250,000 5.99, 131,072 6.03, 65,536 5.06, 16,384 5.03, 4,096 5.51, 1,024 6.50. The arms do not separate: the spread of one arm across runs (up to 1.45x) is larger than the spread between arms (1.29x). "4,096 is 1.7x SLOWER end to end" and "250,000 is the fastest of the sizes measured" are both gone. This is not the arm bias of #5: the PRE-FIX benchmark run on this machine in the same session gives 250,000 at 7.06 and 4,096 at 7.05, so the earlier 5.3-against-9.2 simply does not reproduce here. The constant stays at 250,000 for want of a reason to move it, not because moving it was measured to cost. I do not take the reviewer's statistical argument for this one. For a min-of-three benchmark the minimum across whole runs is the right statistic and overlapping ranges do not by themselves defeat it; what defeats it is that the minima do not reproduce. 8. The transpose-only section modelled a different loop: it read `col[0]` and dropped each column, so 386 of every 387 stores were dead and the allocator returned the same hot buffer every iteration, where `flush_block` holds all 387 live. It now collects and holds them. Cost of the correction, same machine: 2.32 against 1.80 s/Mrow at 250,000, 1.32 against 1.08 at 16,384, 0.18 against 0.18 at 1,024. 9. The lifetime note. `from_iter_values` borrows where `StringArray::from(Vec<String>)` consumed, so the flat text stayed alive through `write` and `close`. Not a regression (the net is smaller) but free to fix: `write_scored_table` drops the two `FlatStr` and the label bits once the arrays own their copies. The panic precondition the same change introduced -- `from_iter_values` is `data_len.expect("Iterator must be sized")` -- is now documented on `FlatStr::iter`, because a `filter` there would turn a write into an abort. OUTPUT EQUALITY The review's strongest point is that the equality evidence touched no handoff code: `ci/smoke.sh` uses `configs/examples/native.json`, so `classifier` is `native_tda`, `rescore.python` is `None`, `stream_to_handoff` is false and `run_pin_sidecar` is never entered, while the one handoff test compared the new writer against itself. Fixed: - `the_flat_metadata_columns_write_the_same_handoff` keeps the pre-change `Vec<String>` writer verbatim in the test module -- `StringArray::from(Vec<String>)`, `protein[start..end].to_vec()`, `label[i] == "decoy"` -- and byte-compares both encodings over 12 rows with a block boundary at 5. Identical. - `the_dense_group_reduction_reproduces_the_hashed_one` gains `group_stride` 9 at 4,000 rows (span 12,002), which is precisely the band whose ARM the gate change in #2 moves, compared bit for bit against the gate-free hashed reference in both q modes over four populations. - `the_flat_column_reports_capacity_and_shrink_returns_it` pins that `bytes()` is capacity and that `shrink` moves no value. End to end: `ci/smoke.sh` at f68a166 and at this commit, each with its own privately copied release binary, both SMOKE_OK; all 104 `.parquet` and `.tsv` artifacts across the six output directories byte-identical, including every `psms_scored.parquet`. The smoke fixture does not reach the gate band (284 rows, max `base_peptide_id` 874, so span 875 takes the direct arm under both gates) or the handoff, which is what the two unit tests above are for. Validated from rust/mumdia: `cargo fmt --check`, `cargo clippy --workspace --all-targets -- -D warnings`, `cargo test --workspace` (318 + 80 passing, 1 ignored), and `bash ci/smoke.sh` printing SMOKE_OK. Out of scope and unchanged: the reviewer's rider to narrow `target_decoy_q` to `(&[f64], &[bool])`, which is in `fdr.rs`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adversarial review of 3f1a991 raised twelve issues. Ten hold and are fixed; two are rejected below with the arithmetic. The list view and the three decoder fast paths (changes 2 and 3) survive unchanged in logic; only the write side moves. THE ONE THAT MATTERED (review issue 2). The shipped rule disabled the dictionary on FLOAT and DOUBLE leaves, which keys on physical type when the discriminator is CARDINALITY RELATIVE TO THE CHUNK. Measured (bench_dictionary_rules_by_column_shape, 1,000,000 rows, 131,072-row groups, dictionary on -> off): constant f64 0.005 -> 0.380 MB (+8,130%), binary 0/1 f64 0.011 -> 0.381 (+3,363%), 0..20 count f64 0.041 -> 0.391 (+855%), 1,001-valued quantised f32 0.529 -> 0.576 (+9%). CLAUDE.md records 10-11 constant columns among the 387 Extended features, plus indicator and count features carried as f64, so the engine writes exactly these. writer_props now sets a per-column dictionary_page_size_limit on each float leaf instead, at half of what that leaf's own values would weigh in the chunk (rows * leaf_bytes / 2). Below that cardinality a column keeps the dictionary and is byte-identical to what the parquet defaults wrote; above it the chunk falls back to PLAIN as before. The leaf WIDTH is load-bearing: a limit sized for f64 is 1.5x a whole f32 chunk, so an f64-sized constant would silently do nothing for predicted_intensity, irt and every other scalar f32 leaf. Scored over the 92 real artifacts of ci/smoke.sh, each rewritten at its own row-group size, against the parquet defaults: dictionary disabled on float leaves -0.93% worst artifact +18.7% 16 KiB fixed limit on float leaves +0.48% worst artifact +30.7% 16 KiB limit on EVERY leaf +1.39% worst artifact +30.7% c = 0.25 -1.09% worst artifact +17.4% c = 0.5 (shipped) -2.24% nothing regressed c = 0.75 -2.19% nothing regressed REJECTED, issue 1: "one rule serves every shape -- a single global set_dictionary_page_size_limit(16 KiB) measured strictly better". It is not strictly better; it is the worst of the seven rules tried on real artifacts, +1.39%. The review's probes cover cardinality 2 and 3 and near-unique and conclude "no regression", but the band between is where the engine lives: a 20,000-accession protein column over 1,000,000 rows goes 3.337 -> 9.091 MB, +172% capped and +340% uncapped, because 20,000 accessions need about 320 KB of dictionary and a 16 KiB limit discards it after the first thousand. Against that it buys 0.4 MB of 485.6 on the features-shaped table, 0.08%. The id-column gain it also cites (unique i32 -33%, run-length i32 -40%) is real and is left on the table deliberately: a per-column INTEGER rule could take it, and it is unmeasured on the real library tables. REJECTED, the headline: this rule is worth about half of what the disable was. Features-shaped, 200,000 x 390 at the 65,536-row cap: disable -18.5%, this -10.1%. The ceiling is structural. The parquet fallback is prefix-based, so the pages written before it fires keep their dictionary and a rule that waits to see the data can never recover what a rule that decided in advance would have. The -13.2% on features.parquet and -20.9% on chromatograms.parquet in 3f1a991 are real, and they are the price of being wrong by two orders of magnitude on any float column that turns out to be low-cardinality. Expect roughly half of them. Also fixed: - Issue 3, false mechanism. "Every row-group cap in this engine is at or below that, so a near-unique float column could never trigger the fallback" ignored write_table, write_batches and BatchWriter::new, which pass no cap and inherit DEFAULT_MAX_ROW_GROUP_ROW_COUNT of 1,048,576; rescore.rs writes psms_scored.parquet through BatchWriter::new. The new rule is a fraction of the chunk, which for an uncapped chunk exceeds the 1 MB default, so it declines to set anything rather than raising it: uncapped files are byte-identical, pinned by an_uncapped_write_is_byte_identical_to_the_parquet_defaults. - Issue 4, the chromatograms number was taken with 1,028,155 rows in one row group where extract writes 16 (CHROM_ROW_GROUP_ROWS). bench_rewrite_a_real_artifact now takes the cap from the source file's own metadata, MUMDIA_BENCH_ROW_GROUP overrides, and the cap is printed with every number. - Issue 5, the write-time claims. "write 1.6 -> 0.54 s (-66%)" was a single shot whose baseline moved 57% between two runs of the same arm. Every write and read delta is dropped from both docstrings; only sizes are claimed. - Issue 6, arithmetic. "about 1.0 GB off the six-run HYE competed table" is wrong: 2,603,894 PSMs x 327 B = 0.85 GB. 1.02 GB is the six-run ASTRAL pool's 3,133,636 rows. Both are now named. The 0.5 GB handoff figure checks out (3.84 GB / 2,485 B = 1.545 M rows x 327 B = 505 MB) and the handoff does use HANDOFF_ROW_GROUP_ROWS. - Issue 7, arithmetic. The zstd extrapolation applied -59%, measured off-against-off (171.6 -> 70.4 MB), to a 136 GB figure that is the dictionary-on footprint. Against dictionary-on snappy (216.8 MB) it is 70.4/216.8 = 32.5%, so about 44 GB, not 60 -- and only to the extent the band artifacts compress like a chromatogram table, which the wide feature and competed tables in that 136 GB do not. - Issue 8, scope. The repetitive-float shape CLAUDE.md warns about is now measured: a 5-modforms-per-peptide iRT column is 3.314 -> 2.242 MB (-32%) capped. The fragment library is the case that decided the rule: its mz is 7,002 distinct over 22,920 rows, which a disable inflates 18.7%. - Issue 9, undeclared behaviour change. ListF32::of downcasts the child eagerly, so a list column with zero rows or every row null whose inner type is not f32 is now an error where the per-row path produced empty rows. Kept, because failing on the schema beats silently returning empty traces, but declared on ListF32 and pinned in a_non_f32_list_is_refused. The commit's "byte-identical by construction" was wrong about this one place. - Issue 10, pub enum ListF32 { Small, Large } became a struct with private fields. No caller in this repository changed, but the variants were published surface of mumdia-io 0.4.0; noted on the type. - Issue 11, test gap. the_decoder_fast_paths_agree_with_the_per_row_loops_on_sliced_arrays runs push_bool, push_str_eq and push_opt_f64 over seven slice windows of a six-row array, including windows that exclude the null so null_count() flips to 0 and the arm changes, comparing against the per-row loops bit for bit. - The SpliceWriter note from the review's output-equality section: a pooled table built from pre- and post-change bands carries both encodings in different row groups. Legal parquet, reads correctly, but reproducible from neither binary alone. Issue 12 is correct and needs no change: 3f1a991 does not touch mumdia-core/src/schema.rs, and neither does this. Superseded and removed: bench_float_dictionary_on_a_wide_float_table, whose "off" arm called writer_props(schema, None) and is now a no-op, together with its write_with and read_secs helpers. bench_dictionary_rules_on_a_features_shaped_table replaces it with the real column composition, every rule and both row groups. EQUALITY. Values-identical everywhere. Bytes move only for artifacts written through a CAPPED writer whose schema has a float leaf -- the spectra tables from convert, chromatograms from extract, features, psms_competed, the rescore handoff, and the library and pool tables -- so their content hashes change, the position MUMDIA_PARQUET_COMPRESSION already documents. Every non-float column chunk is identical byte for byte and the uncapped writers, psms_scored.parquet among them, do not move at all; both are asserted. ci/smoke.sh prints SMOKE_OK and its peptides.tsv and proteins.tsv hash f0b5dc38... and 5a65d304..., the same values 3f1a991 recorded for the PRE-change binary. Validated from rust/mumdia: cargo fmt --check, cargo clippy --workspace --all-targets -D warnings, cargo test --workspace (401 tests), cargo build --release --locked, and ci/smoke.sh against a private copy of the binary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The IO layer turned `ListF32` from an enum into a struct that resolves the offset width and the child once, and exposes `row_slice`; the features change had hand-rolled the same borrow as a local `list_row` matching on the variants. The local copy goes and its five call sites use the shared accessor. One behavioural consequence of the struct, which the IO change declared: a list whose inner type is not f32 is now refused when the column is OPENED rather than when a row is read, because the downcast happens once in `of` instead of per row. The test that pinned the old error site now pins the new one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two CI failures on this branch, both mechanical: - Rustdoc (`-D warnings`) rejected three public doc comments that link to private items. `Evidence::ref_profile_full` linked `weighted_reference_full` and `build_evidence`, and `run_with_chunk_limits` linked `CHUNK_PSM_ROWS`; all three are private. They become plain code spans, which is what the surrounding prose already uses for private names. - `docs/24_config_reference.md` records each environment read by source line, and the rescore-stage and IO work moved those lines. Regenerated; the diff is line numbers only, no setting, default or variable changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ci/check_doc_refs.py` reported `0..md` as a dangling document reference. The match comes from `(0..md.num_row_groups())` in the new `table.rs` benchmark: the filename pattern allows a dot inside the stem, so the range operator plus the binding name reads as `<stem>.md`. No filename holds two dots in a row, so matches containing `..` are skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Twelve analysts surveyed the engine, a skeptic re-derived every finding's arithmetic and dropped what did not hold, five owners implemented what survived, a second reviewer tried to refute each implementation, and each owner then answered that review. This is four of those five subsystems; rescoring is still in its fix round and will follow.
What the review pass was worth, in the four items it caught that testing had not:
-0.0 >= 0.0passes the sorted check while its bits sort after every positive float. Same rows, different integration bounds. The parallel plan now reproduces the single pass exactly, proven over 20 comparisons at 1, 8 and 64 rows per group, at default threads and at one.debug_assertguarded the reference profile's length; in release a short profile silently zeroed one feature family, skipped a peak scan and shortened a correlation. Both readers now check and rebuild.features::runis itself called from inside a rayonpar_iterwhengroups.parallelorexperiment.parallel_runsexceeds 1, so eight decoders meant 32 concurrent row groups at four bands. A process-wide lease makes it 11.What each subsystem does now
features decodes the confident-bounds pass on up to eight threads instead of one (4.63x on this host), borrows trace rows out of the decoded batch instead of allocating an
Arcper row (2.76x), bounds a chunk by PSM rows as well as chromatogram rows, and builds the full-window reference profile once instead of twice.quant reads the chromatogram table by row group in parallel (3.1 to 4.4x in the shipped regime, re-measured after the first benchmark turned out to plan 64 readers where production plans 8), replaces the accepted-candidate
HashSetwith a bitset, and reads the scored table's strings in the shape they are used. Its net memory is honestly up, by about 100 to 145 MB on a stage holding a 355 MB store.the rescore stage reduces the label column to a boolean at the read, narrows the grouped-q key, and sizes its buffers to their contents. Its own benchmark result did not reproduce: the 1.7x block-size effect is smaller than one arm's run-to-run spread, so the claim is gone and the constant stays for want of a reason to move it.
the IO layer replaces "disable dictionary encoding on float leaves" with a per-column cardinality limit, because the physical type is the wrong discriminator. Scored over 92 real artifacts: disabling gives -0.93% with one artifact 18.7% larger, a global 16 KiB limit gives +1.39%, and the shipped rule gives -2.24% with nothing regressed. A 20,000-accession protein column would have grown 172% under the reviewer's own proposal.
Equality
Every subsystem re-ran
ci/smoke.shagainst a privately copied binary and compared all 104 artifacts to its parent;peptides.tsvandproteins.tsvhash unchanged throughout. Where bytes deliberately move, they are named: the chunk cap above one row group, and float columns whose cardinality now keeps or loses a dictionary.One merge conflict worth recording: the IO layer turned
ListF32from an enum into a struct exposingrow_slice, and features had hand-rolled the same borrow. The local copy is gone, and a list with a non-f32 inner type is now refused when the column is opened rather than when a row is read, because the downcast happens once.332 tests,
clippy -D warningsclean, SMOKE_OK.🤖 Generated with Claude Code