mzSpecLib support, packed IonAnnot, and the mzcore migration - #93
Open
jspaezp wants to merge 27 commits into
Open
mzSpecLib support, packed IonAnnot, and the mzcore migration#93jspaezp wants to merge 27 commits into
jspaezp wants to merge 27 commits into
Conversation
These three were committed unformatted; `task fmt` reformats them on any run. Separated out so they do not obscure the real diffs that follow.
rustyms (snijderlab) and mzcore (rusteomics) are separate crates, not a rename, so this is a real port: `pro_forma` now takes an explicit `&Ontologies` and returns `(Peptidoform, Vec<Warning>)`, `SimpleModificationInner::Mass` gained two fields, `ModificationId::id` is private behind an accessor returning `mzcv::AccessionCode`, and `formulas()` moved to the `AmbiguousMolecule` trait. Masses and UNIMOD ids are bit-identical before and after. Ontologies are behind a `OnceLock` reached only from the fallback past the byte-walk fast path. Building them costs ~210 ms / ~200 MB, and a real DIA-NN `.speclib` load never touches it: peak RSS 10.4 MB, zero inits. micromzpaf loses its rustyms dependency entirely. `IonAnnot::from_fragment` and `TryFrom<FragmentType>` had no callers -- the crate now needs no chemistry stack at all (serde + thiserror). The two new tests cover ontology name resolution, which had no coverage: the existing differential tests only assert the fast path agrees with the slow one, and never exercise a named mod that requires a lookup.
Fragment labels must be unique within a precursor -- `linear_get` is first-match, so a duplicate hides data rather than erroring (`ExpectedIntensities::try_from_pairs`). Unknown ions are distinguished only by an ordinal used as a counter, and every site that advanced that counter could wrap: - diann_io (TSV + parquet) and spectronaut_io used `+= 1` on an inferred `u8`: panics in debug, wraps in release past 255. - skyline_io used `saturating_add`, which pins every ordinal past the limit to 255 -- the same collision, failing more quietly. - `try_fill_labels_u8` built labels with `(0..n).map(|i| i as u8)`, which wraps at 256 into `0,1,2,..`. This one is live: it is how JSON inputs without explicit labels get labelled. All four now go through a `checked_add` helper that fails at the row, where the index is still in hand, instead of downstream with a duplicate-key error pointing at the symptom. Also fixes `try_fill_labels_annot` building placeholders at isotope 1, which would have marked every synthesized fragment as the M+1 peak. That path has no callers today. The capacity test is at exactly 256 rather than near it: the first attempt at the fix used `(0..num_fragments as u8)`, which is an *empty* range at 256, and only a test at the boundary catches that.
Libraries spell the same loss differently. Two real collisions in the HUPO-PSI corpus: -CH3SOH (NIST) == -CH4OS (SpectraST) C1H4O1S1 -NH2-CO-CH2SH (NIST) == -C2H5NOS (SpectraST) C2H5N1O1S1 Keying on the string would give `y5-CH4OS` and `y5-CH3SOH` two distinct labels for one ion -- the wrong direction, since fragment labels must be unique within a precursor and a spurious second label hides a real duplicate. So parsing goes text -> composition -> discriminant. Summing terms also collapses `H2O-NH3`/`NH3-H2O` and `2H2O`/`H2O-H2O` for free. Composition is parse-time only; an `IonAnnot` will store the discriminant, so there is no per-annotation cost. The vocabulary is scoped to what the supported inputs actually emit -- DIA-NN none, Spectronaut two, NIST eight -- plus the phospho losses, which no corpus measurement could have surfaced because every PSI example file is non-phospho. SpectraST's wider combinatorics are deliberately absent. `from_expression` distinguishes Ok(None) "valid composition, not in the table" from Err "not a loss expression": callers route the first to an unknown label and the second to a parse failure. Display emits a canonical spelling, so `-CH3SOH` renders as `-CH4OS`. Byte-identical round-trip is therefore not a property of this type and tests compare parsed values.
…fragments
A spectral library carries one annotation per fragment, so this type is
replicated millions of times and compared on the scoring hot path. Packing
it into a single word makes equality one compare instead of an enum
discriminant match plus payload:
linear_get over 13 fragments, worst case
struct of fields (before) 16.3 ns
packed u32 (after) 5.5 ns
The alternative -- adding a `loss` field to the existing struct -- costs
more than the byte it looks like. `IonAnnot` is align-1 but pairs with an
`f32` in `ExpectedIntensities`, so a 5-byte key pads to a 12-byte tuple
and the inline TinyVec storage goes 104 -> 156 bytes. Packed stays at 8
and 104.
Layout is 30 of 32 bits: kind 4, charge 4 (zigzag), isotope 4 (zigzag),
loss 6, payload 12. The payload is a tagged union -- an 8-bit ordinal for
backbone ions, two 6-bit endpoints for internal fragments, a 5-bit residue
for immonium. Bit 30 is reserved for a future registry-backed loss so an
esoteric addition does not force another layout change.
BREAKING (in-workspace only):
- charge is now +-7 and isotope +-7, down from the full i8. Observed
maxima across the HUPO-PSI corpus are 3 and 3, with no negative charges.
Bit fields truncate rather than wrapping loudly, so every constructor
range-checks and `every_field_round_trips_at_its_extremes` exercises the
boundaries -- that is the test that catches a missing check.
- Ord is now by packed word, not field-by-field. Only tests sorted these.
- Display is no longer byte-inverse to parsing: a non-canonical loss
spelling renders canonically, so `y5-CH3SOH` and `y5-CH4OS` are one
annotation. That is the point -- two labels for one ion would fake a
duplicate and defeat per-precursor label uniqueness.
Also adds the mzPAF mass-error suffix (`/-0.0005`, `/1.2ppm`). mzSpecLib
peak lists carry observed m/z, so a reader needs the error to recover
theoretical. `TryFrom<&str>` discards it; `parse_mzpaf` keeps it.
Unrepresentable annotations fail rather than degrade: an unlisted loss,
and modified immonium (`IC[Carbamidomethyl]`, which carries an arbitrary
mod string). Parsing `y1-HCOOH` as plain `y1` would put a loss peak's m/z
on the y1 label and collide with the real y1.
mzSpecLib specifies how to write a CV term, not which term to use, so every field needs a fallback ladder. The two reference exports disagree on almost everything: precursor m/z DIA-NN MS:1000744 Spectronaut MS:1003208 retention time DIA-NN (absent entirely) Spectronaut MS:1000896, minutes ion mobility both MS:1002476 (a drift time, not 1/K0) RT is unit-tagged and the unit is honoured rather than assumed -- reading Spectronaut's minutes as seconds would be a silent 60x error. Peak lists carry OBSERVED m/z; the annotation's mass-error suffix is what recovers theoretical (`theoretical = observed - error`). Two consequences shape the policy: - A theoretical mass only exists once a single identity is pinned. `?` peaks and tied ambiguities have no error to subtract, so they are skipped rather than stored at observed m/z -- an arena mixing observed and theoretical masses would be invisible downstream. - "Known" and "representable" differ. `y1-HCOOH` has a known identity and therefore an exact theoretical mass even though IonAnnot cannot spell the loss. Those peaks are kept with an unknown label: the mass stays exact, only the label is erased. Ambiguous annotations resolve to the smallest absolute mass error. If the winner is unrepresentable the peak takes an unknown label rather than falling back to a worse-matching representable alternative, which would assign both a wrong identity and a wrong mass. Everything that does not land verbatim is tallied and reported once per library. A consensus library carries thousands of unannotated peaks and a line each would bury the signal. Fixtures are the HUPO-PSI examples verbatim (Apache-2.0, same as this project): the DIA-NN export because it is the shape our own writer will emit, and the Spectronaut one because it carries the `-H2O`/`-NH3` losses the packed IonAnnot now represents.
Carafe drives timsquery_cli as a subprocess and parses the output with fastjson -- no field remapping, no schema negotiation. A renamed field or a dropped serde alias fails loudly on neither side: Carafe gets a null and NPEs somewhere unrelated. Keeping the assumptions in another repo made that a landmine. The tests use the literal payloads from the document, through the public entry points Carafe actually reaches (a file path into `read_library_file`, a JSON blob into `Tolerance`), so a refactor that keeps the internals working but moves the boundary still fails. Two things worth knowing that the tests now enforce: - `precursor`, `fragments` and `fragment_labels` are serde aliases of names used elsewhere in the codebase. "Unifying" them would break Carafe while passing every other test. - Carafe writes `percent`/`absolute` where the CLI's own templates write `pct`/`da`. Both spellings must keep working, so both are covered. The `ms` window is `[itol - itol_shift, itol + itol_shift]` -- a +-itol window recentred on a measured calibration offset, not a symmetric tolerance. The test pins both edges distinctly, since collapsing them would quietly recentre every extraction window. Not covered, because it needs the built binary and a real .d: aggregator names, the -o directory layout, and the results.json basename.
Removes the MessagePack reader, the `SpeclibWriter` (whose only arm was
msgpack+zstd), the `.msgpack{,.zst,.zstd}` extension detection, and the
rmp-serde dependency. `.msgpack` paths now fall through to the timsquery
bridge and fail there rather than being claimed by a native reader.
BREAKING: `speclib_build_cli` does not compile. It was the sole user of
`SpeclibWriter`, and its replacement writer emits mzSpecLib -- landing
separately. Left broken deliberately rather than propped up with an
interim ndjson writer that would be deleted a commit later.
Everything else in the workspace builds and passes: timsquery, timsseek,
micromzpaf and the remaining binaries are untouched.
Apex-finder bench
Sensitivity + timing across canonical scenarioscommit f157283 |
Three behaviour fixes found while re-reading the diff: - `resolve_annotation` dropped alternatives whose mass-error suffix failed to parse, which could silently turn an ambiguous peak into an unambiguous one and resolve it against a partial set. A malformed suffix on any alternative now makes the whole peak unresolvable. It also reported that case as `SkipUnannotated`, miscounting an annotated peak as unannotated. - Tie detection compared with `f64::EPSILON`, which is neither exact nor scale-aware. These errors come from parsing decimal literals, so equal ones compare exactly; the fuzzy compare bought nothing and hid the intent. - A malformed ion-mobility value fell back to 0.0. Absent still means 0.0 (DIA-NN writes that), but present-and-unparseable now drops the spectrum rather than inventing a mobility. Also collapses the double scan for the minimum (count-then-find, with an expect between them) into one pass, moves the loss discriminant match next to the enum it must stay in sync with, and drops a `debug_assert` in the `loss()` getter guarding an invariant no constructor can violate plus one asserting a bound the type system already guarantees. `REGISTRY_BIT` went with them -- an unread constant enforces nothing, and the bit reservation is documented in the layout. Trims comments that restated the module docs verbatim; inline comment density is now in line with the neighbouring readers.
Blockers: - `cargo build` was broken for everyone: speclib_build_cli is in default-members and does not compile without the msgpack writer. Dropped from default-members until the mzSpecLib writer lands, so `cargo check` is clean and only an explicit `--workspace` hits it. - Serializing a default-constructed IonAnnot panicked. `Default` is not optional here -- `tinyvec::Array` requires `Item: Default`, `TimsElutionGroup` stores labels in a `TinyVec`, and `KeyLike` propagates the bound -- and `Serialize` renders through `format!`, so the panicking `Display` arm was reachable from any serde path. Renders inertly now. (Pre-existing on main, not introduced here.) Duplication: - `next_unknown_ordinal` was triplicated verbatim; hoisted to `serde/unknown_ordinal.rs` with each caller mapping into its own error. - `IonSeriesOrdinal` had grown copies of `IonAnnot`'s own `terminality` and `try_get_ordinal` that could drift. Parsing also round-tripped `"y12" -> IonSeriesOrdinal -> as_char_and_ordinal -> Kind` for no reason; it now reads the series char directly, which lets the whole duplicate surface go. `IonSeriesTerminality` had no users at all. - The `pro_forma` warning-dropping shim was repeated 3x; now `sequence::parse_proforma`. - Two fixture helpers and three test modules in mzspeclib_io collapsed to one. - mzSpecLib and `.speclib` bypassed the reader registry with hardcoded `if sniff_x` branches. `LibraryReader` gained a `read_arena` default, so every format now goes through `registry()` and adding a direct-arena format no longer means another branch. Dead code: `try_fill_labels_annot` (no callers), `SerSpeclibElement:: sample`/`sample_json` (pub, so no dead-code warning fired; only consumer was the deleted writer test), `NeutralLoss::composition` (now test-only), and the `Composition`/`INTERNAL_POS_MAX` public surface narrowed to the crate. `IonParsingError::Custom`'s one use claimed "exceeds i8 range" for a +-7 field; the range check below it already reports the real bound. Also: msgpack leftovers in speclib_build_cli's `--help`, config default and example TOML (users following those would have produced an unloadable file); the unreachable-by-intent RT unit arm now counts unknown units instead of silently assuming minutes; the 8-field anonymous tuple is a named `ArenaRow`; `from_discriminant` is covered for every variant; and the Carafe tests assert on `Tolerance` fields rather than `Debug` strings, with the missing-`id` case built as its own payload rather than by string surgery on the const.
`speclib_build_cli` was left uncompilable when the msgpack `SpeclibWriter` was removed, and then taken out of `default-members` to hide it. That did not contain the damage: release.yml builds `speclib_build` on all four targets, `--workspace` was broken for every cargo command, and the crate's mzcore port had never been type-checked because rustc aborted at name resolution. `SpeclibWriter::new_ndjson_zstd` is the inverse of the `SpeclibReader` that already existed, so what it emits is what the loader reads back. Config, CLI help, README and run.bash go back to ndjson; they advertised mzSpecLib, which nothing in the tree can write. Replace when the real writer lands. Also here, same file: `SpeclibReader` dispatched between `NdJsonReader` and itself once msgpack left, so it boxes a `dyn BufRead` rather than a `dyn Iterator` and absorbs the reader it was forwarding to. Its blank-line skip recursed per line and could overflow the stack; it loops now. The remote-output tempfile suffix is fixed rather than derived from the destination URI -- `Path::extension` on `lib.ndjson.zst` yields `zst`, dropping the part that names the format. Deletes a committed `mzspeclib_io.rs.bak` (an in-place-sed artifact) and ignores `*.bak`.
…ropped
The bit-packing work introduced `Kind` as a second discriminant enum beside
`IonSeriesOrdinal`, leaving six hand-maintained tables over the same 14
cases with nothing enforcing they agree -- and tests covering only 6 of the
series, so transposing two arms would mislabel a whole ion series silently.
`IonSeriesOrdinal` now carries `to_parts`/`from_parts`/`from_series_char`
and `Kind` is gone. Two new tests walk every variant, through the packed
word and through its mzPAF spelling.
`try_get_ordinal` goes back to an exhaustive match, so a new series cannot
silently get `None` from a storage-shaped predicate.
`UnknownIonCounter` replaces `next_unknown_ordinal`, which was
`u8::checked_add` with a paragraph attached while the actual ~15-line block
stayed triplicated. Five call sites become one line each and overflow is
unrepresentable rather than something each reader remembers to check. The
mzSpecLib reader had hand-rolled its own copy next to the helper.
`LibraryReader` had two mutually-recursive defaulted methods with an
`unreachable!()` standing in for a type-level requirement -- on an exported
trait. One required method returning `LibraryArena`; the five legacy
readers wrap their own result.
`MzSpecLibStats` reported "all clean" on a file that had silently dropped
spectra: `convert_spectrum` bailed on three structural failures and
incremented nothing. Counters now come from one macro-declared list that
`anything_to_report` and the log line both derive from, malformed spectra
get their own counter, and overflow is no longer filed under
"duplicate label" (whose `try_new` arm was dead anyway).
`resolve_annotation` returns the mass error inside the variants that keep
the peak instead of a parallel `Option` that was meaningless for the two
skip cases. `OrdinalOutOfRange { ordinal: -1 }` becomes `MissingOrdinal`.
`ParsedAnnotation`/`parse_mzpaf` and `IonParsingError::Custom` had no
callers; `flush` was a closure capturing nothing; the line loop's
`in_peaks` bool is a `Section`.
`ontologies()` rebuilt what mzcore already ships as `STATIC_ONTOLOGIES`, a `LazyLock` over the same `init_static`, adding only a `tracing::debug!`. Its doc also claimed it was reached only from `parse_sequence_mzcore`; two other call sites reach it through `parse_proforma`. `parse_proforma` returned `Option`, so `count_carbon_sulphur_in_sequence` lost mzcore's diagnostic and reported only the sequence. It returns `Result` now; the two callers that cannot use the message discard it explicitly. Not changed, deliberately: most of the ~200 MB is GNOme glycan data that `modification_to_mod` discards, and mzcore can build a Unimod-only index -- but dropping an ontology also drops the sequences that reference it, turning a mod this code already ignores into a peptide it cannot parse. Noted where it would be done.
GNOme is 191_529 entries / 26.4 MB of the 27.8 MB mzcore loads, and the build goes from ~2.6 s to ~48 ms without it (debug, measured). The earlier reasoning for keeping it was wrong. It assumed a glycan mod was merely ignored, so that dropping the ontology would turn a usable peptide into an unparseable one. It is not ignored: `modification_to_mod` returns `None` for anything non-Unimod and `parse_sequence_mzcore` propagates that `None` for the entire peptide. GNOme's only effect was to let a `[GNO:...]` sequence parse and then be discarded one step later. PSI-MOD, XL-MOD and RESID stay (~1.4 MB combined) so the formula path in `count_carbon_sulphur_in_sequence` still sees them. The one behavioural difference is that a GNO-accession glycopeptide now takes the averagine isotope fallback instead of a composition envelope -- the documented path, already tallied as `n_averagine_fallback`. The new test pins the reasoning rather than the footprint: Unimod (named, numeric, bare mass) still resolves, the three remaining ontologies still reach mzcore and are still rejected downstream, and `[Glycan:HexNAc]` is untouched because a composition needs no index.
The README's build example named the output `vimentin.ndjson`, but the writer always emits zstd, so the resulting file could not be read back. `mzpaf_with_intensities` still inlined the length check and seal that `finish_mzpaf_arena` now owns.
…pack removal `classify_mod` sliced a library-supplied bracket body at a fixed byte index, which aborts the search when byte 7 lands inside a multi-byte char. `SpeclibReader` now takes the compression from the zstd magic number instead of the file name, which removes `SpeclibFormat` and lets a mislabelled file still load. A leftover `.msgpack.zst` reported "stream did not contain valid UTF-8"; it now names the removed format and how to rebuild. `FileReadingError::path` is an `Option`, so the two sites that had no path say so instead of printing an empty one, and zstd failures get their own variant rather than being laundered through `serde_json::Error::io`.
`u8` was the first arm of both JSON ladders, so numeric or absent `fragment_labels` always produced `TinyIntLabels`, which `from_elution_groups` rejects. The whole integer half could only ever return `Err`, including the label-synthesis helper and its two tests. Unlabelled input now fails as `MissingFragmentLabels` instead: a label names a series and an ordinal, and a positional index is not one. `try_read_json` propagates that instead of flattening it to `UnableToParseElutionGroups`.
…None`
The nine backbone series differed only by a letter but were spelled out in
four hand-mirrored tables (`to_parts`, `from_parts`, `from_series_char`,
`Display`) — ~36 of ~52 arms carrying no information, kept in agreement only
by a round-trip test. They are now one `Series` enum with one letter table,
and each of those four sites drops to five arms.
That gives `FragmentLabel` the `series()` accessor its own TODO asked for.
`IonSeriesOrdinal::None` existed to satisfy a `Default` bound that
`#[derive(Default)]` on the `u32` newtype already provides; it had been a
field of `IonAnnot` before the packing change, and was not one after. Instead
`unknown` takes discriminant 0 and charge is stored biased by one, so the
all-zero word — which `tinyvec` can hand out at any time — is `?0` at charge
1. `IonAnnot::default()` is now a real annotation that parses back, rather
than a value only `Display` accepted.
The packed layout is not a wire format: `IonAnnot` serializes as its mzPAF
string, so the charge bias changes no file on disk.
Also names `INTERNAL_POS_BITS`/`IMMONIUM_BITS` — the two field widths had
four unnamed copies between them — and covers both at their ceilings, which
`internal { start: 2, end: 11 }` never reached.
…the tally `[n]` group ids are block-scoped in mzSpecLib, but every block's attributes landed in one bag keyed on the id alone. `spectronaut.mzSpecLib.txt` already uses `[2]` for the RT+unit pair in its Spectrum block and for the NCBI TaxID in its Analyte block; it works only because the latter carries no unit term. Reading the wrong unit is a silent 60x error in the RT. `find` now also prefers the Spectrum block instead of trusting file order, and attributes in an unapplied `<AttributeSet>` block are counted rather than dropped silently. The tally had three defects. A malformed spectrum was reported as malformed AND as whatever the RT/mobility counters had already recorded on the way to the bail-out. A collided peak was counted as both kept and dropped. And a DIA-NN library warned "9 spectra using a drift time as mobility" on every load, because DIA-NN writes `0.0` there to mean "unset" — so the clean-load branch was unreachable for the format. Annotated peaks with no `/error` suffix were stored at observed m/z with no counter, which is the one case that produces the observed/theoretical mixture the module header forbids. Now counted. A malformed suffix reports as malformed rather than as ambiguous. Documents that `MS:1003072|spectrum origin type` — which declares whether the peak m/z are observed at all — and the `DECOY` attribute set are both unimplemented, rather than leaving the header's blanket claim standing.
Three readers had three answers to "labels must be unique within a precursor": the mzSpecLib reader dropped the later peak, the DIA-NN `.speclib` reader collapsed onto the more intense one, and `mzpaf_with_intensities` — which the DIA-NN/Spectronaut/Skyline TSV readers all feed — did nothing at all. The third is not "fails later". `ExpectedIntensities::try_from_pairs` rejects duplicate keys and timsseek's scoring pipeline `.expect()`s it, so two TSV rows with the same series + ordinal + charge for one precursor panic mid-search, per candidate. Nothing upstream of that dedupes, and the `HashMap<IonAnnot, f32>` intensity lookup silently collapsed such rows on the way through. `FragmentSet` now owns the invariant and the parallel `(labels, mzs, intensities)` bookkeeping for all three, collapsing onto the more intense peak: intensity is what gets scored, and file order is not meaningful. Each caller keeps its own counter name for the collision.
`ReferenceEG.id`, `.precursor_labels`, `.precursor_intensities` and `PrecursorEntry.decoy_group` were serialized and then dropped by the loader: `push_row` receives none of them, and the isotope argument is a literal `&[]`. The loader recomputes the envelope from composition (`IsotopeStrategy::FromComposition`) regardless. Producing `precursor_intensities` was not free — it was the only consumer of `count_cs_modified`, so every entry paid a second full ProForma parse plus a formula computation on top of the one `compute_precursor_mz` already does. That is roughly half the mzcore work in a Koina build, spent on bytes nobody reads. `count_cs_modified` is gone; `compute_precursor_mz` was already the malformed-sequence gate the deleted step claimed to be. Serde ignores unknown fields, so existing libraries still load. Also drops `speclib_build_cli`'s `strip_mods`, which had no callers outside its own test and handled only `[...]`, unlike the two real copies.
…ests The contract test covered only the input direction, and its header claimed the output half needed the built binary and a real `.d`. That is false for the field names, the aggregator names and the format name. Demonstrated: adding `#[serde(rename)]` to a `SpectrumOutput` field — invariant 3, the one that silently NPEs on Carafe's side — left the whole suite green. The output assertions live inside `timsquery_cli` because it has no library target. They deserialize the contract's own payloads into the real types and compare the emitted key sets, so no constructor is needed and a rename fails. `results.json` is now a named const so the basename is assertable. `cli_template_tolerance_spellings_still_deserialize` was a byte-for-byte copy of `WIDE_TOLERANCE_TEMPLATE` duplicating a test that already deserializes the actual constant the CLI writes; deleted. So was the tolerance re-serialize round trip, which cannot fail — `Tolerance` is only ever read on the Carafe path and `#[serde(alias)]` is deserialize-only, so both sides move together under any rename. `mz_range`'s panic message asserted "low and high are positive". Carafe emits a negative low when the calibration offset exceeds the half-width; the real invariant is `low + high >= 0`. Corrected and covered. Also drops the `AIGear.java` line-number citations, which go stale on any Carafe commit above line 6660.
The previous version round-tripped the boundary types through serde and compared key sets. That pins the type's shape, not what Carafe reads: the whole path from `-f ndjson` through `JsonStreamSerializer` to the file was untested, and contract invariant 2 — one complete object per line, no array wrapper, no pretty-print — was not asserted anywhere. Records now go through the same serializer `stream_process_batches` uses and the assertions are on the resulting text. Verified by mutation that all three of these now fail and previously would not have: dropping the ndjson newline separator, swapping ndjson to `to_writer_pretty`, and renaming a field via `#[serde(rename)]`. Also pins that the non-ndjson formats DO wrap in an array and that the default format is not ndjson, so `-f ndjson` is demonstrably load-bearing rather than incidentally correct, and that an empty result set is an empty file rather than a truncated one.
…e offender
Three near-identical `strip_mods` copies differed only in which bracket pairs
they recognised — DIA-NN's and timsseek's missed `{...}`, so a Spectronaut-
style mod would survive into a "stripped" sequence. One implementation now
handles all three pairs and is covered for nesting and unbalanced closers.
The mzSpecLib reader took its stripped sequence from `MS:1000888` and fell
back to `""`, which is not a peptide with no mods, it is a zero-residue
peptide: the row silently took the averagine isotope path and was tallied two
layers away, where the cause was no longer visible. That term is optional in
mzSpecLib, so the stripped form is derived from the proforma and counted.
`ElutionGroupCollection`, `FileReadingExtras`, the per-format
`*PrecursorExtras` and `LibrarySniffError` were `pub use`d from `serde` with
no consumers anywhere. Un-exporting them surfaced dead members the `pub` had
been masking: an unused `len`, an extras slot on `StringLabels` no reader
fills, and three sniff-error payloads that were formatted nowhere.
`LibrarySniffError` now has a `Display` and the Spectronaut sniff logs it —
`MissingColumns` names the columns a near-miss export lacks, which is the
difference between "wrong format" and "wrong export settings".
One unparsable row anywhere disables sequence-derived scoring for the entire
library; the only record was an `info!` with no offending sequence. It is now
a `warn!` that names the row and the usual cause.
`LibraryReader::read` was four bodies differing only in the function called, the log string and the `FileReadingExtras` variant — and since `read` started returning `LibraryArena`, all four built an `ElutionGroupCollection` only to destructure it in the same call. Each is now one line through `arena_from_pairs`. That deletes `FileReadingExtras`, whose three variants existed to be immediately flattened into `PrecursorExtrasRow`, and lets both `ElutionGroupCollection` variants drop their extras slot. Those four `map_err`s also flattened every reader-specific failure into `UnableToParseElutionGroups`, which made `read_library_file`'s keep-the-first-error rule preserve nothing — the variable was still called `last_err`. Failures now carry the reader name and its own error, and the variable matches the rule.
`mzcore` takes `default-features = false`: its `flate2` default is reachable only through `CVIndex::init()`, which we never call. The `mzcv` entry now records why it is a direct dependency at all and what breaks if mzcore is bumped without checking — `cargo tree -d` confirms one shared mzcv today. `ontologies()` was a `OnceLock` behind a wrapper fn while the same file used `LazyLock` for `AA_COUNT_NAMES`; now a `LazyLock` static like its neighbour. Comment trims. The "Task 9"/"Task-4"/`speclib_data_flow.md` references point at planning documents that are not in version control, so they name nothing a reader can follow; replaced with what the code does. Also drops comments arguing for a test's own value, one recording which input the author tried first, and one explaining what deleted code would have done — except in `or_averagine_falls_back_on_nonstandard`, where "`X` does not work here" is a real trap for the next person to touch that fixture, so it stays, reworded. Test trims in `loss.rs`: `phospho_losses_resolve` asserted three TABLE rows the table round trip already covers, and `NeutralLoss::composition` was a `#[cfg(test)]` method whose only use asserted TABLE against itself. The formula test claimed to cover implicit counts but every count in it was a single digit; it now covers multi-digit counts and the `u8` boundary, which nothing exercised.
Nine tests hand-rolled the same six-line path to the sibling crate's test data, and `expect_lazy` was an identity function documented as existing to avoid churning ten call sites — churning them is the smaller cost. The two native-NDJSON tests wrote to `env::temp_dir()` under a PID-derived name and cleaned up with a `remove_file` placed after the assertions, so a failing assertion leaked the file and two concurrent runs of the same test binary collided. `tempfile` was already a dependency.
This was referenced Aug 27, 2026
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.
Makes mzSpecLib the canonical spectral-library input, and removes msgpack.
speclib_build_clidoes not compile on this branch. It was the only userof
SpeclibWriter, whose sole arm was msgpack+zstd. Its replacement writeremits mzSpecLib and lands separately. Everything else builds and passes.
Read the commits in order — each is self-contained and the later ones only
make sense given the earlier ones.
What is here
pro_formanowtakes an explicit
&Ontologies. Masses and UNIMOD ids are bit-identical.Ontologies sit behind a
OnceLockreached only past the byte-walk fastpath, so a real DIA-NN load never builds them (10.4 MB peak, zero inits).
micromzpafdrops its chemistry dependency entirely — theFragmentTypeAPI it existed for had no callers.
precursor;
linear_getis first-match, so a duplicate hides data insteadof erroring. All four counter sites could wrap. One is live: JSON inputs
without labels went through
(0..n).map(|i| i as u8).IonAnnotpacked into au32.linear_get16.3 ns -> 5.5 ns. Adding aplain
lossfield instead would cost 3 bytes, not 1: paired with anf32a 5-byte key pads to a 12-byte tuple, growing the
ExpectedIntensitiesinline storage 104 -> 156 bytes. Gains neutral losses, internal fragments
and bare immonium.
-CH3SOH(NIST) and-CH4OS(SpectraST) are one loss; so are-NH2-CO-CH2SHand-C2H5NOS.String keys would give one ion two labels and fake a duplicate.
reference exports disagree on precursor m/z, and DIA-NN omits RT entirely
while Spectronaut writes minutes.
docs/, with tests pinning the fieldnames and tolerance aliases against the literal payloads in the document.
The one design call worth reviewing
mzSpecLib peak lists carry observed m/z; the mass-error suffix recovers
theoretical. So a theoretical mass only exists once a single identity is
pinned, which splits peaks three ways:
y1-HCOOH)?) or tied ambiguityThe third row is skipped rather than stored at observed m/z: an arena mixing
observed and theoretical masses would be invisible downstream. Ambiguous
annotations resolve to the smallest mass error, and if that winner is
unrepresentable the peak takes an unknown label rather than falling back to a
worse-matching representable alternative.
On the PSI corpus this retains 100% of DIA-NN and Spectronaut peaks; the
losses are confined to NIST/SpectraST and are dominated by unannotated peaks.
Also worth knowing
cargo fmton stable fights this repo —imports_layoutandimports_granularityare nightly-only, so it collapses the vertical importsused everywhere. Use
task fmt. The first commit folds in pre-existing driftin three files that were committed unformatted.