feat(micromzpaf): pack IonAnnot into a u32 and add neutral losses by jspaezp · Pull Request #104 · TalusBio/timsbuktoolkit

jspaezp · 2026-08-28T16:31:14Z

PR 2 of the #93 carve-up. Hard prerequisite for the mzSpecLib reader (PR 5), which cannot parse its fixtures' -H2O / m3:7 annotations without these constructors.

Why packed

IonAnnot becomes a packed u32 so that the annotations an mzSpecLib-shaped library needs -- neutral losses, internal fragments, immonium ions -- fit without growing the type.

The argument is about the alternative, not about the predecessor. The old representation was already 4 bytes (a 2-byte series+ordinal enum plus two i8s), so this is not a shrink. But bolting a loss field onto that struct would have cost three bytes, not one: a 5-byte key paired with an f32 pads to a 12-byte tuple, which at timsseek's inline capacity of 13 grows ExpectedIntensities's inline TinyVec storage from 104 to 156 bytes. Packed, the tuple stays 8 bytes and the loss rides in bits nobody was using.

Neutral losses are keyed by composition, not spelling

Libraries write the same chemical loss different ways (-CH3SOH in NIST, -CH4OS in SpectraST). Since fragment labels must be unique within a precursor and lookup is first-match, keying on the string would hide a genuine duplicate behind two spellings. A loss outside the table is rejected rather than coerced onto a nearby representable ion, so a loss peak's m/z can never land on the bare y5 label.

Side effects worth calling out

UnknownIonCounter replaces a counter four sites each hand-rolled. Skyline's used saturating_add and ElutionGroupInput's cast an index, so past 255 both reissued a label already handed out -- making every later peak carrying it unreachable. Both are now errors.
IonSeriesTerminality is deleted -- terminality() had no callers. Series takes its slot in the re-export and resolves the FragmentLabel TODO that was waiting on exactly that type.
IonAnnot is deliberately not Ord. The packed word sorts by ordinal, then loss, then isotope, then charge, then series -- not an order anyone means. KeyLike never required it and the three downstream PartialOrd derives had no call sites.
Isotope offsets are unsigned. A signed field let Display emit y5+-2i, which is not mzPAF and reparsed happily -- so a library could round-trip through serde into text no other mzPAF reader accepts, contradicting this crate's own docs. Rejecting negatives makes that unreachable and frees the sign bit, widening the range from 0..=7 to 0..=15.
Charge is capped at ±7 (4 zigzag bits) where it was i8. Corpus maximum is 3. Every constructor range-checks, because a bit field truncates silently.

rustyms leaves micromzpaf; from_fragment had no callers. It stays a dependency of timsseek and speclib_build_cli, which is the mzcore migration's problem.

Not done here

diann_speclib_io.rs is the one reader holding neutral-loss data and it still drops it (152 fragments on the pinned fixture), because DIA-NN's loss code byte needs a numbering this PR does not have. Guessing it would mislabel peaks, which is worse than dropping them. Filed as #105; the comment there no longer claims IonAnnot cannot represent losses.

split_mass_error/MassError also have no caller yet -- they ship with the parser they belong to, and PR 5 is the first consumer.

Verification

cargo test --workspace, both feature-gated cargo checks, -p timsseek --no-default-features, -p calibrt -p calib_dash, -p timscentroid --features mzdata all green. cargo +nightly fmt --check clean. Clippy holds at 7, the too_many_arguments set tracked in #32. RUSTDOCFLAGS="-D warnings" cargo doc -p micromzpaf now passes -- it failed on main.

The layout is self-checking: const assertions cover total width, abutting shifts, two internal endpoints per payload, the immonium alphabet, both zigzag bounds, and that the loss table has not outgrown LOSS_BITS.

IonAnnot becomes a packed `u32` instead of a struct of fields, so equality is one word compare and `(IonAnnot, f32)` stays 8 bytes. A spectral library carries one annotation per fragment, so the type is replicated millions of times in a loaded arena. The new capacity is spent on the annotations an mzSpecLib reader needs: neutral losses, internal fragments, bare immonium ions, and the mass-error suffix. Losses are keyed by atomic composition rather than by spelling, because libraries write the same chemical loss different ways (`-CH3SOH` in NIST, `-CH4OS` in SpectraST) and fragment labels must be unique within a precursor. Losses outside the table are rejected, never coerced onto a nearby representable ion. Drops the rustyms dependency: `from_fragment` had no callers. `UnknownIonCounter` replaces the counter three readers each hand-rolled. Skyline's used `saturating_add`, so past 255 unknown ions it reissued `?255` and made every later peak carrying that label unreachable. `IonSeriesTerminality` is gone -- nothing called `terminality()`. `Series` takes its place in the re-export and resolves the standalone-series-enum TODO in `FragmentLabel`.

github-actions · 2026-08-28T16:35:58Z

Apex-finder bench

cargo run -p apex_sim --release --example bench -- 1000 2

Sensitivity + timing across canonical scenarios

=== summary (n=1000, tol=±2 cycles) ===
  scenario                      pass2%  pass1%   medErr    us/run
  clean                          100.0   100.0        0     27.87
  moderate_noise                  92.7    92.7        0     27.41
  high_noise+interference         59.3    59.3        1     27.31
  heavy_interference               7.4     7.4       88     27.84
  mismatched_library              59.8    59.8        1     27.23
  absent_top_fragment             16.0    16.0       65     27.23
  absent_precursor                59.3    59.3        1     27.27

=== broad apex-finding (n=500, tol=±2 cycles) ===
  scenario                      pass2%  pass1%   medErr    us/run
  broad_clean                    100.0   100.0        0    161.26
  broad_moderate_noise            50.6    50.6        1    157.85
  broad_high_noise+interf         27.6    27.6      308    162.10
  broad_hard_3x_density            0.8     0.8      487    156.94
  broad_mismatched_library        41.0    41.0      161    156.90
  broad_measured_density          49.6    49.6       13    186.85

=== narrow recovery (n=1000, tol=±2 cycles) ===
  scenario                      pass2%  pass1%   medErr    us/run
  narrow_clean                   100.0   100.0        0     18.72
  narrow_moderate_noise           81.3    81.3        0     18.60
  narrow_high_noise+interf        65.8    65.8        1     18.74
  narrow_hard_3x_density          10.9    10.9       34     18.59
  narrow_mismatched_library       62.8    62.8        1     18.54
  narrow_measured_density         83.6    83.6        0     21.49

=== narrow score discrimination (AUC, n_seed_pairs=1000) ===
  scenario                         AUC   med+signal    med-noise
  narrow_clean                   1.000     4.329e14      7.199e7
  narrow_moderate_noise          0.870      4.054e9      3.054e8
  narrow_high_noise+interf       0.830      2.761e9      3.602e8
  narrow_hard_3x_density         0.684      3.644e9      1.244e9
  narrow_mismatched_library      0.842      2.172e9      3.575e8
  narrow_measured_density        0.858      4.341e9      2.562e8

commit ab85106

jspaezp · 2026-08-28T17:53:27Z

+use crate::IonParsingError;
+
+/// Slots in [`Composition`], in the order [`Composition::new`] takes them.
+const C: usize = 0;


can we save this by making it a struct?

jspaezp · 2026-08-28T17:53:48Z

+/// from C/H/N/O/S/P, and keeping it to six `u8`s makes equality a single
+/// 6-byte compare during the parse-time table lookup.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
+pub(crate) struct Composition([u8; 6]);


Q: why not a struct?

the slot could even be indexed on the enum ... like get_mut(&mut self, char) ??

Review follow-ups, all mechanical. The crate doc led with a false claim: the predecessor was already 4 bytes, so `size_of` pins nothing on its own. The argument is against the alternative -- bolting a loss byte onto the old struct would have made a 5-byte key that pads to a 12-byte tuple, growing timsseek's inline storage 104 -> 156. The test now asserts the added fields cost no space rather than restating a size that never changed. Isotope offsets become unsigned. `ISOTOPE_MIN = -7` let `Display` emit `y5+-2i`, which is not mzPAF and reparsed happily -- so a library could round-trip through serde into text no other reader accepts, contradicting the crate's own doc. Rejecting negatives makes that spelling unreachable and frees the sign bit, widening the useful range from 0..=7 to 0..=15. `const` assertions replace the prose contract: total width, abutting shifts, two internal endpoints per payload, the immonium alphabet, both zigzag bounds, and that the loss table has not outgrown `LOSS_BITS` (`pack` masks the discriminant, so a 64th loss would decode as a different one). `TABLE` becomes the index for `from_discriminant` and `canonical`, deleting a 13-arm hand-mirrored match and turning two linear scans into `TABLE.get`. The drift the old round-trip test guarded against is now one invariant. `IonAnnot` loses `Ord`. The packed word sorts by ordinal, then loss, then isotope, then charge, then series, which is not an order anyone means; `KeyLike` never required it, and the three downstream `PartialOrd` derives had no call sites. `UnknownIonCounter` loses `Copy`/`Clone`: a forked uniqueness counter reissues labels, which is the bug it exists to prevent. `ElutionGroupInput::try_fill_labels_annot` mints through that counter instead of casting an index, so running past the label space errors rather than wrapping into duplicates; it also no longer stamps isotope 1 onto every placeholder. `try_fill_labels_u8` rejects >255 fragments for the same reason. `ParsingError.context` was an `Option` that was never `None`, and two sites reported the wrong thing: a bare `I` as a *modified* immonium, and `p1` as an out-of-range ordinal for a series that takes none. Fixes the two `cargo doc` failures: public docs linked private items.

jspaezp · 2026-08-28T17:58:00Z

Q: why not update the examples with the things that are representable now?

jspaezp · 2026-08-28T17:58:11Z

+//! at all. Because a bit field truncates rather than wrapping loudly, every
+//! constructor range-checks -- see [`IonAnnot::try_new`].
+//!
+//! # mzPAF compliance


pretty over-stated .. and not that useful TBH

jspaezp · 2026-08-28T17:58:47Z

+//! internal fragments, immonium ions -- fit *without* growing the type.
 //!
-//! # mzPAF Format Compliance
+//! That is the whole argument, and it is about the alternative rather than


"That is the whole argument, and it is about the alternative rather than" is useless in this comment

PR review notes. `Composition` becomes a struct of named `u8`s instead of `[u8; 6]` plus six index constants, so `TABLE` reads as chemistry: `H2O` is `h: 2, o: 1` rather than `Composition::new(0, 2, 0, 1, 0, 0)`, where a transposition is invisible on review and would alias one loss onto another. The symbol-to-field mapping now lives once, in `count_mut`, which the parser indexes instead of keeping its own list of slots. Rows use a local `C!` macro: a plain struct literal has to spell all six counts, which is the positional noise the struct is meant to remove, and `..ZERO` is not allowed bare in a `const` item. Crate doc: drop the sentence announcing what the argument is instead of making it, and retitle "mzPAF compliance" -- this parses a subset, so the heading overstated it. The examples now show a neutral loss with its two spellings, an internal fragment, an immonium ion, the mass-error suffix, and two rejections, rather than two variations on `b12`.

Reader errors said "DIA-NN precursor parsing error" and nothing else. The `From<IonParsingError>` impl that flattened them had no way to see the row, so it is replaced by a `map_err` adaptor that stamps one on, and the three `*ReadingError`s now carry the precursor error instead of discarding it. A bad row aborts the whole load, so the row index is the only handle the user gets. `CHARGE_MAX` is 8, not 7. The bias means the field holds `-7..=8`; capping at 7 rejected a charge the encoding can represent. The `const` assertion is what keeps this honest, and it is why `CHARGE_MIN` cannot follow suit: `-8` zigzags to 17 and truncates to 1, decoding as charge 0. The `.speclib` fragment charge is a raw wire byte and was cast with `as i8`, turning anything above 127 into a plausible negative charge. It is range-checked now. The sibling `typ()` is masked `& 0x7F` for DIA-NN flag bits; if this byte carries flags too, this is where it surfaces (#105). The isotope-offset failure said the situation "should never happen" instead of what the limit is. It now names the fragment and the representable range. `TryFrom<&str>` still drops the mass-error suffix -- the suffix belongs to one observed peak, not to the ion, and storing it would make two `b12`s unequal -- but it warns once per process rather than doing it silently. `cargo doc --workspace` now passes and has a `task doc` target. It was failing in five crates: ten links to private items, and ten "broken" links that were never links at all (`[U:4]`, `["Carbamidomethyl@C"]`, `[0,1]`, `[Apply]`). Also drops `sort_vecs_by_first!`, which had no callers outside its own tests.

`Display for IonSeriesOrdinal` emitted `m2:11` and `IA`, but the code parsing those forms was inlined in `IonAnnot::parse_ion` -- an inverse pair split across a type boundary. Likewise `INTERNAL_POS_MAX` was checked in `IonAnnot`, 400 lines from the 6-bit packing that makes 63 the right bound; the `debug_assert` in `pack` was a debug-only backstop for an invariant two unrelated functions maintained by hand. `series` now owns all three: the spelling (`Display` next to `parse`), the payload layout (`to_parts` next to `from_parts`), and the bounds that layout imposes (`try_internal`, `try_immonium`). It exports `KIND_BITS`/`PAYLOAD_BITS` so the word can be assembled without knowing how they are filled, and asserts its own payload bounds. `to_parts` is total now -- every arm masks to its own width, so the immonium `- b'A'` cannot underflow for a variant built by hand. `IonAnnot` keeps two constructors instead of four: `new(series, loss, charge, isotope)`, and `try_new(char, ordinal, charge, isotope)` for the shape a file reader actually has. `try_new_with_loss`, `try_new_internal` and `try_new_immonium` are gone. `parse_ion` is now the three suffixes peeled in the one order that is unambiguous, then a delegation, and the module doc draws which delimiter belongs to which stage -- previously that ordering was load-bearing and only the `-` case said so. `lib.rs` drops 1210 -> 705 lines, with `error.rs` and `parse.rs` taking the rest. Tests moved with their subjects: the packing and spelling round trips are in `series`, where they can see the encoding they are pinning. The constructors are `try_internal`/`try_immonium` rather than `internal`/`immonium` because a constructor sharing a name with its variant is ambiguous to rustdoc, and reads ambiguously to a person too.

…ping them The `.speclib` reader discarded every fragment carrying a neutral loss -- 152 on the pinned fixture -- because `IonAnnot` could not represent one. It can now, so the only thing missing was DIA-NN's loss-code numbering, which is undocumented. Measured it instead of guessing. A lossy fragment and its no-loss sibling differ by exactly the loss, so `(no_loss_mz - lossy_mz) * charge` is the neutral mass. On the fixture, code 1 gives 18.011 across 47 pairs and code 2 gives 17.027 across 37: water and ammonia, with nothing else within half a dalton. The two account for all 152. `loss_codes_are_water_and_ammonia` re-derives that from the fixture rather than trusting the table, and fails on any code this build does not map -- so a DIA-NN version that renumbers them stops the load instead of relabelling peaks. An unmapped code is still dropped and counted, because a wrong loss puts a real m/z on a label that collides with a different real fragment.

jspaezp commented Aug 28, 2026

View reviewed changes

jspaezp mentioned this pull request Aug 28, 2026

diann_speclib_io drops neutral-loss fragments: no DIA-NN loss code -> NeutralLoss map #105

Open

jspaezp commented Aug 28, 2026

View reviewed changes

jspaezp added 6 commits August 28, 2026 11:06

chore: trim PR archaeology comments

967e3c2

style: cargo fmt

1021be6

jspaezp merged commit bf6f667 into main Aug 28, 2026
5 checks passed

jspaezp deleted the feat/packed-ion-annot branch August 28, 2026 19:51

Navigation Menu

Uh oh!

feat(micromzpaf): pack IonAnnot into a u32 and add neutral losses - #104

feat(micromzpaf): pack IonAnnot into a u32 and add neutral losses#104
jspaezp merged 8 commits into
mainfrom
feat/packed-ion-annot

jspaezp commented Aug 28, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 28, 2026 •

edited

Loading

Uh oh!

jspaezp Aug 28, 2026

Uh oh!

jspaezp Aug 28, 2026

Uh oh!

jspaezp Aug 28, 2026

Uh oh!

jspaezp Aug 28, 2026

Uh oh!

jspaezp Aug 28, 2026

Uh oh!

jspaezp Aug 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

jspaezp commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why packed

Neutral losses are keyed by composition, not spelling

Side effects worth calling out

Not done here

Verification

Uh oh!

github-actions Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Apex-finder bench

Uh oh!

jspaezp Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

jspaezp Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

jspaezp Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

jspaezp Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

jspaezp Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

jspaezp Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jspaezp commented Aug 28, 2026 •

edited

Loading

github-actions Bot commented Aug 28, 2026 •

edited

Loading