Conversation
Clear the ground before restructuring, so the moves in the following commits are readable. Output is unchanged. - the commented-out `df.explode` fallback in `merge_precursor_fragment_df`, superseded by `explode_multiple_columns`, which handles the same old-pandas case it was guarding against - two commented-out column assignments, `LabelModifiedSequence` and `ProteinGroups` - `DiannParquetCols.SIGNATURE`, declared but never written; it is referenced only by the docstring saying DIA-NN wants it omitted, which stays - the six `frag_*_head` arguments of `speclib_to_single_df`, which only passed their own defaults through to `merge_precursor_fragment_df`. No caller passes them, in alphabase, its notebooks, peptdeep or alphadia; the parameters on `merge_precursor_fragment_df` itself stay, since the DIA-NN export uses them. - the pandas-version branch picking `to_csv`'s newline argument, duplicated in `WritingProcess.run` and `translate_to_tsv`, hoisted to one module constant. Kept rather than dropped: `requirements_loose.txt` does not pin pandas, so a pre-1.5 install is still possible. Verified with a scratch harness comparing 15 outputs -- 7 transition-list parameter combinations, 4 DIA-NN combinations, 2 tsv batchings and 2 parquet batchings -- against the same functions loaded from main: all identical, frames compared including dtypes and files by digest. Test files are untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`merge_precursor_fragment_df` took the half-built *output* frame, so both formats copied `frag_start_idx`/`frag_stop_idx` into it just for the flattener to read and then dropped them again, and the DIA-NN export smuggled a precursor pointer through it to flag base peaks. Split it in two: `fragment_table` takes the index arrays and returns one row per kept fragment in canonical columns plus `precursor_row`, and `join_fragments` repeats each precursor row across its fragments. Each format now names the fragment columns with a rename dict instead of `frag_*_head` arguments, so a third dialect is a dict literal. Output is unchanged. `fragment_table` still ends in `explode_multiple_columns` on purpose: building the columns from typed arrays would change the exported dtypes, which belongs to the fixes PR. `merge_precursor_fragment_df` is removed; no caller exists in alphabase, peptdeep or alphadia. `DIANN_PARQUET_FRAG_HEADS` is renamed to `DIANN_FRAGMENT_COLUMNS`, and is in no released tag. Verified with the scratch harness: 15 outputs identical to main, compared including dtypes and index. Test files are untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The m/z window zeroed intensities in the caller's library instead of dropping fragments, so an export edited what it was handed, exporting twice at different windows was order-dependent, and empty fragment slots (m/z 0) leaked into the output when the window was disabled -- `translate_to_tsv` at 0/0 wrote nothing else. Filter inside `fragment_table` on the per-precursor copy it already made, and read precursor m/z through `get_precursor_mz` instead of writing it onto the caller's frame. `mask_fragment_intensity_by_mz_` and `mask_fragment_intensity_by_frag_nAA` go with the design they implemented. An unbounded window is now expressed by its own bounds, 0 and `np.inf`, rather than the 0/0 sentinel, which warns and is treated as unbounded. Output is unchanged except where the window is disabled: of 18 outputs compared against main, the four that differ are exactly those calls. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mo-sameh
changed the title
mo-sameh
changed the title
mschwoer
reviewed
Sep 9, 2026
mo-sameh
marked this pull request as ready for review
September 17, 2026 07:19
mo-sameh
requested review from
GeorgWa
and
a lite review from Copilot
and removed request for
GeorgWa
September 20, 2026 15:35
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The refactor removes documented public API names and parameters without a compatibility path.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (3)
What changed in this PR
This PR extracts shared spectral-library export logic into translate_core.py, keeping SWATH and DIA-NN format-specific schemas and writers separate.
Changes:
- Adds shared fragment selection, sequence rendering, and precursor helpers.
- Refactors both exporters to avoid mutating source libraries.
- Updates tests and documentation for the new filtering behavior.
| File | Description |
|---|---|
translate_core.py |
Shared export machinery |
translate.py |
SWATH exporter refactor |
translate_diann.py |
DIA-NN exporter refactor |
test_translate.py |
SWATH regression coverage |
test_translate_diann.py |
DIA-NN regression coverage |
translate.rst |
SWATH documentation update |
translate_diann.rst |
DIA-NN documentation update |
translate_core.rst |
New core-module documentation |
modules_spectral_library.rst |
Documentation index update |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+5
to
+6
| names it used to define are re-exported below, so importing them from here keeps | ||
| working. |
Collaborator
Author
There was a problem hiding this comment.
They are not imported outside of this module in alphabase, peptdeep and alphadia ?!
This branch has not been deployed
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
translate_diann.pyimported five helpers fromtranslate.py, which made the SWATH format the de facto shared library for the DIA-NN one. Move what both formats share into a newtranslate_core.pyso neither depends on the other's dialect.What's here
translate_core.pyholds the modified-sequence rendering, the candidate precursor columns, and the fragment flattening. Each export keeps only its own column names and writer, and renames the canonical fragment columns into its own dialect through a dict.The flattening is where the real change is. It used to filter by zeroing intensities in the caller's library and letting a later
min_frag_intensitydrop the rows. One design choice, four symptoms: a default export silently zeroed 60% of the library's intensities, a second export at a wider window returned different fragments than a fresh one, the export wrote aprecursor_mzcolumn into the caller's frame, and disabling the m/z window exported empty fragment slots at m/z 0. Fragments are now selected on a per-precursor copy and dropped rather than zeroed, which fixes all four.Verification
Every export was run against
main's modules over 19 configurations and compared including dtypes and index. The first four commits are byte-identical tomainon all 19, with no test edits. The fifth differs on exactly the four cases that disable the m/z window — the fix — and the characterization tests that pinned the old behaviour change with it.pytestwarnings drop from 41 to 7; theSettingWithCopyWarningstorm was the export writing into batch slices of the caller's library.