Conversation
dia_sample_level_modifications copied the frame and merged it with the SDRF sample table on Run: the categorical key upcast to object for every row plus an object Sample column - the same transient already measured in the intensity plot, and the stage (mod_plot_dict) the 72 GB run died in after 8 minutes once everything upstream was fixed. run_to_sample_codes maps Run->Sample through the category codes: one small array, no strings. Used here and in the per-sample identification counts. Run stays in the per-sample subset so drop_duplicates keeps counting a peptidoform once per run within a sample. A test asserts the result equals the merge-based computation, including runs absent from the SDRF.
plots.dia imported it from dia_utils, and dia_utils imports plots.dia. MultiQC loads dia_utils first, so the partially initialised module made the import fail and the whole QuantMS module was skipped - the report was written without it. A test now imports dia_utils first in a fresh interpreter.
_get_peptide_length ran .str.len() on the categorical Stripped.Sequence column - pandas converts every row back to a Python string, 231 M on PXD030304 - then looped value_counts over 5,798 runs. It runs right after mod_plot_dict with no log line of its own, which is where the 72 GB run was killed 6.5 minutes into that stage once everything before it fit. Length is computed once per distinct sequence and broadcast through the category codes; the per-run histogram is a single groupby size. A test asserts the result equals the string-based implementation.
parse_msstats_input only draws the peptide/protein quantification tables; it stores nothing else. It ran unconditionally whenever the file was found, --disable-table or not. On PXD030304 the staged MSstats input is a 20.6 GiB, 231,620,697-row CSV with six string columns; pd.read_csv took the process from 22 to 93 GB in three minutes and it was OOM-killed - after every DIA stage had completed at ~43 GB - to build tables that --disable-table then discards. Gate the parse on tables being enabled. A test pins the gate.
…tensity std On PXD030304 (5,798 runs) draw_dia_ms1_area passed every raw MS1 area to box.plot and polars panicked serialising a >4 GiB buffer (assertion failed: bytes.len() <= u32::MAX). draw_dia_intensity_std carried 42.7 M points. #718's 'switch to flat plot' changes the renderer, not the point count (#717). summarise_box_data already exists for exactly this and is used by the intensity-distribution box and six other plots: above the flat threshold it replaces each sample's list with {min,q1,median,q3,max,mean}, which MultiQC's box plot accepts directly. Apply it at the two remaining DIA sites. Below the threshold raw points are kept, so small reports are unchanged.
Contributor
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe change adds vectorized DIA run-to-sample and box-statistics helpers, applies size-based aggregation to DIA plots, optimizes peptide-length and modification processing, and skips MSstats parsing when tables are disabled. ChangesDIA processing and plotting optimization
QuantMS MSstats parsing gate
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 5 minor |
| Security | 1 medium 1 minor 20 high |
🟢 Metrics 52 complexity
Metric Results Complexity 52
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
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.
Pull Request
Description
Brief description of the changes made in this PR.
Type of Change
Summary by CodeRabbit
Performance
Bug Fixes
Tests