feat: propagate the library's own precursor id instead of minting one - #98
Merged
Conversation
Apex-finder bench
Sensitivity + timing across canonical scenarioscommit fdccb29 |
This was referenced Aug 28, 2026
An id is an id whether the source wrote an integer or a string, so `SourceId`/`OwnedSourceId` carry either and `#[serde(untagged)]` renders each as what it is. Carafe's `"id": 7` stays a JSON number; a DIA-NN `transition_group_id` will stay a string rather than being coerced into a u64 or replaced by one we made up. `SourceIds` gains a `Text` arm, stored blob+offsets like `seq_strip_blob`, and `SourceIds::owned` keeps an all-numeric library on the dense integer column rather than paying for a blob. The parquet `library_id` and `decoy_group_id` columns become Utf8, format version 3. `HasQueryData::id` is gone -- it returned a u64 nothing consumed. No behaviour change on its own: every id is still Numeric until the readers start propagating.
DIA-NN names every precursor and we were throwing it away, so a search result could not be matched back to a row in the library it came from. Both DIA-NN paths now carry that name through: - `.speclib`: the entry name was already parsed and discarded after the charge suffix was stripped off it. It is the same string as `transition_group_id`. - tsv/parquet: `transition_group_id` is read where present. It is absent from some DIA-NN variants (`sample_lib.txt` has no such column), and those fall back to the minted id, which is what they got before. Spectronaut and Skyline carry no per-precursor name at all, so they keep minting rather than being handed a synthesized one. `Target::reset_from` overwrites the id in place, reusing the `String` capacity: it runs per query on the scoring hot path, where a text id would otherwise allocate every time. That path goes away entirely with the `IsotopeOffset` decorator.
…s a number `cargo check -p timsseek_cli --features calib-dashboard` is a separate CI leg that `--workspace --all-targets` does not build, so it missed the id change the same way it missed the FlatIdx one. calib_dash has no timsquery dependency and its fixtures build entries from plain values, so it takes the id as a `String` and the conversion happens at the timsseek_cli boundary. It only diffs and displays, so text is enough. `CalibrantPoint` gives up `Copy` for `Clone` as a result.
- **The DIA-NN parquet variant was never wired up.** Only the TSV row struct gained the column, so a parquet library still got counter ids while the PR claimed otherwise. DIA-NN 2.2 spells it `Precursor.Id` there, not `transition_group_id`; the Carafe-written fixture has no such column, so both the propagating and the falling-back path now have a test. - **A partially-named file could refuse to load.** A blank id cell made one group fall back to a counter while its neighbours kept names, and `SourceIds::owned` then coerced the whole library to text -- turning `Numeric(7)` into `"7"`, which is exactly the coercion this type exists to prevent, and which can collide with a real name of `"7"`. `owned` now refuses a mixed set outright, and the readers decide once per file: a library names every precursor or none, and a half-named one falls back to minting with a warning. - **The per-query allocation fix was applied in one place and missed two.** The chromatogram and spectrum collectors are reset on the same scoring path as `Target::reset_from` and still built a fresh `String` per query per collector, up to three per scored candidate. They use `set_from` as well now. - **`target_decoy_compete` cloned a group id two or three times per candidate** purely to build comparison keys, across both of its passes. It compares by index. `timsquery_cli` likewise cloned an id on the success path to use it only on the error path. Two tests fill gaps the review named rather than restating what the code says: the speclib ids are checked row-by-row against the property that defines them (modified sequence + charge), since they ride through a rayon fold where a permutation would mislabel every result silently; and `owned` gets the mixed-shape case it had no coverage for.
The readers set `Target::id` from `transition_group_id` / `Precursor.Id` correctly, and then `mzpaf_with_intensities` sealed the arena without carrying it over -- and that is the arm every DIA-NN library takes, because those readers always return `Some(FileReadingExtras::Diann(..))`. `seal()` saw `SourceIds::Absent` and minted `0..n` over the names it had just parsed. Only the `.speclib` path worked, because it writes ids onto the arena itself. Setting the ids and sealing are now one function, so the three arms cannot seal without deciding about ids, which is the shape of the mistake. The tests missed it by asserting on `Target::id()` -- the value the readers produce, one layer above the thing that discarded it. They are replaced by two in `library_file`, at the arena layer, reading back through `read_targets` what a result would actually be keyed by. Reverting the wiring fails them with `["0"]` against `["AAAAAAALQAK2"]`.
`set_source_ids` took `Vec<LibraryId>` and had no callers. The other two differed only in the shape they accepted, so they collapse into one generic over `Into<OwnedSourceId>` -- which is what the two live call sites were reaching for. Also puts them back beside each other; `set_source_ids_text` had been appended 38 lines away, past unrelated accessors.
…ments - `RESULTS_FORMAT_VERSION` was bumped to 3 with no `- 3:` entry, leaving the one thing that tells a reader why an old file misreads stopping at 2. - `source_id`'s module doc claimed no id is ever coerced into the other's shape while the parquet writer coerces both to Utf8. Say where the shape is preserved and where it is not. - `set_source_ids_from`'s doc said the tabular readers store no ids, which this branch is what made false. - Two comments narrated the diff rather than the invariant.
…oy_groups `append_arena` handled columns by hand and silently skipped `source_ids` and `decoy_groups`. Harmless today -- the speclib path sets both on the merged arena, so a shard never holds any -- but the next column added would have been dropped on every merge with nothing to say so. It now destructures `TargetColumns` exhaustively, so adding a column fails to compile here. `decoy_groups` was also missing from `seal`'s shrink list.
Fully-qualified `crate::models::` and `pyo3::` paths inline where a `use` (or the pyo3 prelude) was already there, `Default::default()` at a struct-literal field where the type name says more, and a `let` binding that only existed to name what `#[builder(into)]` already accepts.
…dget `FrameStore::new` sizes its slab from `size_of::<CalibrantPoint>()`, which was exact while the id was a `u64`. Carrying it as a `String` made `size_of` report 40 bytes for a point that costs 40 plus the id text, so both budgets (`REPLAY_BUDGET_BYTES` 1 MiB, `DEFAULT_RUN_BUDGET_BYTES` 64 MiB) under-counted by roughly 40% -- and the slab, preallocated with empty `String`s, grew past the budget as frames filled it rather than being bounded up front. The crate never displays the field: every use is set membership for churn diffing. So it takes an opaque hash of the id, computed at the timsseek_cli boundary. `CalibrantPoint` is `Copy` again, `size_of` is the true cost, and this debugging view stops shaping the production id type.
…in the rule An id keeps the shape its source used, per library: integer in, integer out; string in, string out; a library that mixes them is an error. That was the behaviour already but nothing tested it, so the JSON path silently gained string ids and could have silently lost them again. Pinned in `carafe_contract/`, where a decision about the `id` field belongs: a numeric id still serializes as a bare number (Carafe reads it with fastjson and keys results by it), a text id round-trips as a string, and a mixed library fails to load. `MixedShapes` now names the offending row and value. The check runs over a whole library, so a hand-assembled file with one bad row gave no way to find it. Deciding the shape by collecting `Option<Vec<_>>` also drops the count-then- rematch and its `unreachable!`.
jspaezp
force-pushed
the
feat/source-id-text
branch
from
August 28, 2026 05:43
626dc91 to
52d84bd
Compare
A competition group was carried as an `OwnedSourceId` per row, which is wrong in both directions. Rows that compete *share* a group, so a label per row stores the same string once per member -- the opposite of `source_ids`, where ids are unique and CSR fits. And no consumer reads the value: grouping sorts by it and compares it, nothing else. So a group is now a `GroupCode`: an opaque, `Copy`, `Ord` handle into a deduplicated label set. Sorting and comparing need no arena and no allocation; the label is resolved from the arena only where it is actually written out. Nothing is stored when the input declares no groups, which is every format today. A row is then its own group, so both the code and the label are derived from the row -- `decoy_group_code` returns the row, `decoy_group` returns the row's own id. Previously `seal` materialised a `Vec` of one `String` per row for this, which for a text library meant an allocation per row duplicating a column that already existed. `seal`'s warning now fires only for the case that actually loses information: a library that ships its own decoys and declares no groups, where a stored decoy really does compete alone. It used to fire on every load, including `LazyMassShift`, where minting is simply correct. Handle construction is `pub(super)` throughout, so the arena is the only thing that mints one -- `GroupCode` and `FlatIdx` outright, `RowIdx` bar the flyweight that unpacks it.
A scored candidate carried `library_id` and `decoy_group_id` from Phase 3 to the writer, which then stringified both. Five `String` clones per candidate to move a value nothing on that path read. The scoring path now carries opaque handles -- the arena row plus a `GroupCode` -- and the Parquet writer resolves both ids from the arena at the end. `Identity`, `PeptideMetadata`, `Peptide` and `CalibrantCandidate` each lose their id field; competition keys off `GroupCode`, which is what it always wanted (equality and ordering, never the value). The two id columns move from `Identity::columns` to `FinalResult`, in the same position, because they are no longer on the result. The cost is that a `FinalResult` is no longer self-describing: interpreting one needs the arena it came from. That is fine while writing, and it is what keeps a position from ever reaching an output file. The q-value determinism sort already keyed on the row rather than the id; its doc comment still described the old key. `timsquery`'s new `test-support` feature exposes handle constructors for downstream tests, which assemble results without an arena. It is enabled only through a dev-dependency -- verified that a `cargo build` of the CLI compiles timsquery with `feature="mzdata"` alone -- so a shipped build still cannot mint a handle. Closes review items 3 and 7.
A blank `transition_group_id` in one cell falls the whole DIA-NN library back to minted ids, because the arena holds one id shape and not two. That trade stays, but the warning only reported how many rows were named -- and the fallback then rewrites every id, so nothing was left to find the offending row from. The warning now names the first unnamed precursor and says what the fallback costs. `first_unnamed` is split out so what it reports is testable, and the mixed-shape path has a test at all for the first time.
…ames a slot `Query::new(lib, row, variant)` let a caller pair a row with a decoy variant by hand, which is what `FlatIdx` exists to prevent -- it *is* the name of a (row, variant) pair, and `split_flat` is documented as the single authority for that encoding. The `u8` was never checked against `variants_per_row` either, so naming a variant the library does not expand into read a real but wrong slot. The flyweight now takes a `FlatIdx` and splits it once at construction, so: - nothing outside the arena mints a `RowIdx`; `RowIdx::new` drops from `pub(in crate::models)` to `pub(super)`, the same as the other two handles. - `flat_for` gives the arena the inverse transform, with the range check the loose pair never had. It is now the only way to name a specific variant, which in practice means test fixtures; production only ever goes through `item_at`. The fields stay unpacked rather than deriving from the stored flat, because `row()` is on the path of every geometry accessor and deriving costs a division by a non-const `variants_per_row`. Measured: the flyweight is two words before and after, and `the_flyweight_stays_two_words` now pins that. Also, the `Default` audit: `FlatIdx`'s had no callers at all and is gone. `RowIdx`'s and `GroupCode`'s have exactly one, `Identity::sample_default()`, which `#[derive(ScoreBlock)]` deliberately leaves un-gated; the doc records that instead of the `#[serde(skip)]` reason it used to claim, which was never true (`Identity` only derives `Serialize`).
Clippy is back to main's 19-warning baseline: the branch had added four `iter_nth_zero` warnings in `diann_speclib_io`'s tests, where closing off `RowIdx::new` turned `RowIdx::new(0)` into `.rows().nth(0).unwrap()`. Those go through a `row(geom, i)` helper now. The id columns get a `ScoreBlock` (`parquet_writer::Ids`) instead of two hand-written `o.str` calls in the writer and two matching declarations in `FinalResult::column_schema`. That split was the schema/data drift the derive exists to prevent, reintroduced by hand. `SourceIds`: `LibraryId` is deleted (it had no callers left, and its `#[serde(transparent)]` doc described a wire format with no call site); `numeric`/`text` are private so `owned` is the only way in and the length check lives in one place; `owned` decides the shape from two `position` calls instead of a collect plus two scans plus an `expect`; `text` dedups over the finished blob rather than cloning each id into a `HashSet<String>`. `MixedShapes`' `first_text_free_row` is renamed `first_numeric_row`, which is what it holds. Decoy groups get their own length-mismatch variant, so the message stops calling them source ids. `Serialize` is off the borrowed `SourceId`: the result path writes an `OwnedSourceId` (`ChromatogramOutput.id`), and the contract tests were pinning Carafe's `"id": 7` against the sibling type. They now serialize what production serializes, and the borrowed impl had no other caller. `seal`'s `variants_per_row <= u8::MAX + 1` assert cannot fire (`n_decoys` is a `u8`), and `variants_per_row_for` existed only to serve it from an unbounded `impl`. Both gone; `variants_per_row` is the single definition. Also: `identity_hash` takes `FlatIdx` rather than `impl Hash`, which also accepted `score.to_bits()`; `materialize_peptide` loses the trait-to-inherent forward; three byte-identical determinism-key closures in `qvalues` become one `determinism_key`; two assertions that restated the function body they were checking now assert values; duplicated test arenas share a builder. Three comments this branch's own renames had made false (`calib_dash`'s `metrics.rs` and `frames.rs`, `qvalues`' fixture doc) say what the code does, and the comments that narrated the change rather than the invariant are trimmed to the invariant. `RESULTS_FORMAT_VERSION` now records that `decoy_group_id` equals `library_id` until a format declares groups, so a reader can tell the duplication is expected.
A row could be stored without its name and the names zipped on afterwards by position. That is why `6451904` had a bug to fix: `mzpaf_with_intensities` sealed before attaching the ids, so `seal` minted `0..n` over the names the reader had just parsed. `push_row` now takes a `Row` struct carrying `id: Option<OwnedSourceId>`, and `seal` builds the id column from what the rows arrived with. The whole attach-afterwards layer goes: - `set_source_ids` and `library_file::seal_with_source_ids` are deleted, along with the doc comment that had to explain the bug to justify fusing them. - `push_target` is deleted; `Row`'s `Default` covers the optional half, so the `// precursor_mz` comments annotating positional args at call sites go too. - `.speclib`'s rayon accumulator loses its fifth element and `map_entry` loses a `&mut Vec<String>` out-param -- the names ride with their rows now. - `append_arena` merges ids like any other column, so its "deliberately not merged" carve-out goes. - `Target::set_id`, a `pub(crate)` mutator added only for the post-pass, is gone and `Target.id` is read-only again. `seal` is fallible, because building the id column is what rejects a duplicate or a mixed-shape library. It stays idempotent: timsseek seals a second time after stamping the decoy strategy, and the id column is built by the first call. DIA-NN naming is now decided per file rather than inferred per row. Whether the `transition_group_id` / `Precursor.Id` column exists is read once from the header (`csv::Reader::headers()` caches and consumes no record); if it does, every row must have a name and a blank cell is an error. That deletes `unify_source_ids`, `is_named` and `first_unnamed`, and removes the implicit "numeric means unnamed" encoding -- which held only as long as DIA-NN spells its ids as text, and would have read a named row as unnamed the day one writes integers. A file with no name column is unaffected: minted ids, as before. The native speclib reader was the fifth path and still discarded the id it held. `ReferenceEG::id` and `PrecursorEntry::decoy_group` were both parsed and never read; both are now carried, which also gives `set_decoy_groups` its first non-test caller. `OwnedSourceId::Default` (`Numeric(0)`) is replaced by an explicit `placeholder()`. Every `u64` is a valid id, so a numeric placeholder that leaked looked like a real result keyed on 0 -- the same reason `RowIdx::default()` is `u32::MAX`. It is `Text` so the scratch buffer's first `set_from` reuses the allocation rather than making one. Closes review items 2, 3 and 4.
jspaezp
commented
Aug 28, 2026
| /// RT". | ||
| /// One heap entry, flattened. | ||
| /// | ||
| /// `identity` exists because churn diffing needs to tell "same calibrant, |
Collaborator
Author
There was a problem hiding this comment.
Most of this comment is useless
jspaezp
commented
Aug 28, 2026
| /// decoy geometry is stored anywhere; variants 1/2 compute a ±CH2 mass shift | ||
| /// on the fly from `TargetCapabilities::decoys`. | ||
| /// | ||
| /// A flyweight is built from a [`FlatIdx`] and nothing else, because that is |
jspaezp
commented
Aug 28, 2026
| /// | ||
| /// A struct rather than ten positional arguments, and it carries the row's `id`, | ||
| /// which is the point: a row cannot be stored apart from the name its file gave | ||
| /// it. Attaching ids afterwards, zipped on by position, is how the DIA-NN names |
jspaezp
commented
Aug 28, 2026
| pub seq_strip: &'a str, | ||
| pub seq_mod: &'a str, | ||
| pub mods: &'a [(u8, u16)], | ||
| /// A stored decoy, as opposed to one the arena derives. Default `false`: |
Collaborator
Author
There was a problem hiding this comment.
why is this needed?
jspaezp
commented
Aug 28, 2026
| /// [`Self::append_arena`] like any other, so a shard cannot separate a row | ||
| /// from its name. | ||
| /// | ||
| /// Emptied by `seal`, which is the only reader. |
jspaezp
commented
Aug 28, 2026
| use crate::models::SourceId; | ||
| use std::path::PathBuf; | ||
|
|
||
| /// A file with the `transition_group_id` column promises every row has one. |
Collaborator
Author
There was a problem hiding this comment.
never seen this in reality ...
…llocating
Six comments cut to what a reader cannot get from the code: the `CalibrantPoint`
identity rationale, the flyweight's bit-packing history, `Row`'s account of the
bug that motivated it, `is_decoy`/`pending_ids`/`source_ids` field docs, and the
justification on the blank-name test for a case that does not occur in practice.
`OwnedSourceId::placeholder()` returns `Text(String::new())` rather than
`Text("<unset>")`. An empty `String` owns no buffer, so the two scratch
constructors allocate nothing to store bytes nothing reads -- and the previous
claim that `"<unset>"` let `set_from` reuse the allocation was wrong: a 7-byte
capacity does not hold a real id, so it reallocated anyway.
Repo-wide sweep over `.rs`, `.toml`, `.md` and `.py`, including the two
`\u{2014}` escapes in `calib_dash`'s fit-tab titles that a literal search does
not find. The `calib_dash` UI snapshots are regenerated: the only changes are
the dash itself and the trailing padding that shifts because `--` is two columns
where the em dash was one.
jspaezp
added a commit
that referenced
this pull request
Aug 28, 2026
Three entries described behaviour that no longer exists, and one licensed the bug #98 was about. `Output id` said "the source id where there is one, the arena index otherwise". `seal` mints a source id for every unnamed row, so an arena index can no longer reach a result -- which was the point of the work. The term has collapsed into `Source id` and is gone; "output id" moves to that entry's avoid list. `Decoy group` said "a target and its decoy variants". A declared group is interned by label, so it can span several targets -- the reverse-decoy case the speclib generator emits. Competition keys on `(group, charge)`, so the entry now says one result survives per group and charge, and distinguishes declared from derived groups. `Variant` was defined as a member of a decoy group. It is a member of one row's decoy expansion; under a declared group those are different sets. `Arena index` loses its mechanics. What it means is in `mod index`'s doc and the constraint is enforced by the type -- `pub(super)` construction, no `Display`, no `Serialize` -- so prose repeating it can only go stale. The naming guidance stays, since that is the part no code states.
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.
DIA-NN names every precursor and we were discarding it, so a search result could
not be traced back to the row it came from. Carrying that name through means an id
is no longer always an integer.
An id keeps the shape its source used
SourceId/OwnedSourceIdhold either au64or a string, and#[serde(untagged)]renders each as what it is. Carafe's
"id": 7stays a JSON number; DIA-NN'sAAAAAAALQAK2stays a string. Neither is coerced into the other's shape, and neitheris replaced by a counter we invented.
SourceIdsgains aTextarm stored blob+offsets likeseq_strip_blob, and refusesa set that mixes shapes rather than stringifying the numbers. An all-numeric library
keeps the dense integer column.
What each reader reports now
transition_group_idwhere the column exists, else mintedPrecursor.Id(DIA-NN 2.2's spelling), else minted.speclibbinaryThe
.speclibreader was already parsing that name and throwing it away afterstripping the charge suffix. A library names every precursor or none: a half-named
file (a blank id cell) falls the whole file back to minting with a warning, rather
than producing an arena that mixes shapes.
Spectronaut and Skyline could have had an id synthesized from
ModifiedPeptide+charge, but that manufactures a label in DIA-NN's shape rather than propagating one.
Also here: a target-decoy competition bug
Found while tracing the id through scoring, and folded in because it is the same code
path. In
target_decoy_compete,previouswas pinned to a group's winner while theloop wrote into that winner's slot, so every later member overwrote it. A group of
three — every row under
LazyMassShiftwith two decoys, i.e. every DIA-NN library —left the winner holding its margin over the worst member instead of the runner-up,
and left the members below it holding nothing.
It stayed plausible because the worst member never outscores the runner-up, so the
feature was merely inflated.
delta_group_ln1p_diff/_ratioare#[feat(raw)]model features, so this was quietly feeding the rescorer a wrong number.
The last member of a group keeps
NaN: nothing beneath it to separate from, andColumnTransformimputes non-finite values while emitting an_isnacompanion, so"no rival existed" reaches the model as its own signal.
The existing test only ever built a group of two, which is why this never showed.
Breaking: parquet id columns are Utf8
library_idanddecoy_group_idchange from UInt64 to Utf8, andRESULTS_FORMAT_VERSIONgoes to 3. Anything reading those columns as integers needsupdating. The q-value determinism sort is unaffected — it moved onto the opaque
RowIdxin #95, so it never sees a caller-supplied value.Python sees an
intor astrto match.HasQueryData::idis deleted; it returned au64nothing consumed.Note for the follow-up
Target::reset_fromand the chromatogram/spectrum collectors overwrite the id inplace, reusing
Stringcapacity: they run per query on the scoring hot path. Thatscratch-buffer path goes away with the
IsotopeOffsetdecorator.decoy_groupsis stillVec<OwnedSourceId>, which is the wrong encoding in bothdirections — a per-row copy of
library_idwhen minted, and something that should bedictionary-coded once a generator supplies real shared groups. Left for the work that
adds the first
set_decoy_groupscaller.