[translate 1/3] Characterize the spectral library exports - #444
Open
mo-sameh wants to merge 1 commit into
Open
Conversation
The SWATH/tsv export had no tests and the DIA-NN one only three. Pin today's behaviour before restructuring, so the refactor can be shown to change nothing and the fixes after it to change exactly what they claim. No source changes. Ten tests pin behaviour that is wrong and gets fixed later, each marked CHARACTERIZATION (bug) in its docstring: - both exports zero intensities in the caller's library and add `precursor_mz` to its precursor frame, so a second export at a wider m/z window silently differs from one on a fresh library - with the m/z window disabled, m/z 0 padding leaks into the output; and `translate_to_tsv` masks unconditionally, so 0/0 -- the documented way to disable the filter -- writes a file whose every fragment is at m/z 0 - `rt_norm_pred` is not accepted as a retention time column - DIA-NN `Flags` groups the base peak by `Precursor.Id`, so the duplicate precursors `SpecLibBase.append` produces share a single flag - the SWATH export leaves its fragment columns object dtype with a string `FragmentCharge`, at ~14x the memory of the typed equivalent One more pins `speclib_to_swath_df` returning None -- the evidence that the later commit removing it is safe, since nothing can depend on a function that has only ever returned None. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mo-sameh
changed the title
mo-sameh
marked this pull request as ready for review
September 3, 2026 07:03
There was a problem hiding this comment.
🟢 Approval recommended
The PR is test-only, the assertions are deterministic and well-scoped to current behavior (including explicitly-marked known-bug characterizations), and no correctness or maintainability issues were found in the added tests.
Pull request overview
Adds characterization-focused unit tests for spectral library “translate” exports (SWATH/Spectronaut and DIA-NN) to lock in current behavior—explicitly including several known-bug behaviors—so follow-up stacked PRs can demonstrate isolated, intentional behavior changes.
Changes:
- Introduces a comprehensive SWATH/Spectronaut transition-list export characterization suite (columns/order, mod rendering, RT precedence, filtering behavior, TSV round-trips).
- Extends DIA-NN export tests to characterize RT column precedence,
rt_norm_predrejection,Flagsgrouping behavior, in-place library mutation, and parquet batching determinism.
File summaries
| File | Description |
|---|---|
| tests/unit/spectral_library/test_translate.py | New characterization test suite for SWATH/Spectronaut transition-list export + TSV behaviors and documented buggy edge cases. |
| tests/unit/spectral_library/test_translate_diann.py | Adds additional DIA-NN characterization tests for RT selection, flags behavior, side effects, and parquet batching determinism. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
mschwoer
approved these changes
Sep 9, 2026
Comment on lines
+10
to
+19
| 1. the export zeroes intensities in the caller's library and adds `precursor_mz` | ||
| 2. so a second export of the same library silently differs from a fresh one | ||
| 3. with the m/z window disabled, empty fragment slots leak into the output | ||
| 4. `translate_to_tsv` masks by m/z unconditionally, so disabling the window | ||
| yields a file whose every fragment is at m/z 0 | ||
| 5. `rt_norm_pred` is not accepted as a retention time column | ||
| 6. the exploded fragment columns are object dtype, with a string `FragmentCharge` | ||
|
|
||
| One more, `test_speclib_to_swath_df_returns_none`, pins a function that a later | ||
| commit removes rather than fixes. |
Contributor
There was a problem hiding this comment.
these read a bit weird .. also, remove all refenrences to "later commits"
Collaborator
Author
There was a problem hiding this comment.
Yeah my bad, I added these to make the stack easier to review. They do get rewritten by 2/3 and 3/3 as the bugs get fixed, so this intermediate state won't survive the merge, but I see now how weird it reads on its own.
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
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.
Goal
Pin what the SWATH/Spectronaut and DIA-NN exports do today, so the two PRs stacked on top can be shown to change only what they claim to.
What's here
Tests only — no source changes. A shared fixture builds a library covering N-terminal, C-terminal, internal and loss-bearing modifications, at charges 1–2, so it has both real fragments and empty (m/z 0) slots. On top of that: column names and order, modified-sequence rendering, RT column precedence, the m/z / top-k /
min_frag_nAAfilters,Flags, and tsv and parquet round trips.The 'tsv translate' functionality didnt have unit tests before.
Some of these pin behaviour that is wrong. Those are marked
CHARACTERIZATION (bug)in their docstrings and say which later commit changes them — a test that flips in PR 3 is meant to, and the diff there is the evidence.