Support multi-file analyses and improve logging#3
Conversation
- Fix join to use (file_stem, scan) instead of scan alone, enabling
correct multi-file analyses where scan numbers are not unique across
files
- Parse ms_run->file mapping from mztab MTD header; extract file_stem
from Percolator filename column
- Fix peptide stripping to handle bracket notation with decimal points
(e.g. Q[0.984]) by using x[2:-2] slice instead of split('.')
- Add --fdr argument to control the Percolator FDR threshold used for
calibration (default 0.01)
- Fix bare imports to relative imports so the package works correctly
with python -m glissade.glissade
- Add if __name__ == '__main__' guard
- Write glissade_stats.json with matched/external counts
- Improve logging throughout: PSM counts, FDR pass rates, agreement
rates, min-length filter retention, and unique peptide collapse ratios
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Hoist `import os` to module level (was conditionally scoped inside psms.txt branch, would cause UnboundLocalError in the mztab path) - Guard `spectra_ref` parsing against malformed/missing patterns; helper functions return None/'' instead of raising AttributeError - Guard all percentage logging against zero denominators in align_to_reference() and seperate_scores() - Update read_data() docstring to reflect (file_stem, scan) join key Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Support multi-file analyses and improve logging
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPreprocessing's ChangesMulti-file Join and FDR/Stats Reporting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as glissade.py main()
participant Preprocessing as preprocessing.py
participant FDR as FDR control
participant Output as glissade_stats.json / peptide.tsv
CLI->>Preprocessing: read_data(files)
Preprocessing->>Preprocessing: derive file_stem, join on [file_stem, scan]
Preprocessing-->>CLI: joined dataframe
CLI->>Preprocessing: align_to_reference(df, database_fdr_threshold=args.fdr)
Preprocessing->>Preprocessing: compute in_tide/agrees counts and percentages
Preprocessing-->>CLI: aligned dataframe
CLI->>Preprocessing: seperate_scores(df, min_length)
Preprocessing->>Preprocessing: log matched/external counts before/after filtering
Preprocessing-->>CLI: matched, external sets
CLI->>FDR: run_procedure(n_boots=n_bootstraps)
FDR-->>CLI: q-values
CLI->>Output: write glissade_stats.json
CLI->>Output: write_results() -> peptide.tsv
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR improves Glissade’s ability to run multi-file analyses by fixing the Percolator↔Casanovo join key (now (file_stem, scan) instead of scan alone) and adds more user-facing instrumentation and CLI configurability to make calibration/FDR behavior easier to understand and control.
Changes:
- Join Percolator and mzTab results on
(file_stem, scan)by extracting file identity from Percolatorfilenameand mzTabms_run[..]-locationheaders. - Fix peptide stripping when modification masses contain decimal points by avoiding
split('.')and usingx[2:-2]. - Add
--fdrCLI flag, add__main__guard, emitglissade_stats.json, and expand step-by-step logging.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/glissade/preprocessing.py | Adds multi-file join keys, safer mzTab header parsing, and more per-step logging around FDR/agreement/filtering. |
| src/glissade/glissade.py | Switches to relative imports, adds --fdr, writes stats JSON, adds __main__ entrypoint guard, and improves console logging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _parse_scan(ref): | ||
| m = re.search(r'scan=(\d+)', ref) | ||
| return int(m.group(1)) if m else None |
There was a problem hiding this comment.
Fixed in b798357 — both helpers now coerce ref to str before applying regex, handling NaN/None safely.
| def _parse_stem(ref): | ||
| m = re.match(r'(ms_run\[\d+\])', ref) | ||
| return run_map.get(m.group(1), '') if m else '' |
There was a problem hiding this comment.
Fixed in b798357 — same coercion added to _parse_stem.
| dn_scans = [_parse_scan(x) for x in denovo_df['spectra_ref']] | ||
| dn_stems = [_parse_stem(x) for x in denovo_df['spectra_ref']] | ||
| denovo_df['scan'] = dn_scans | ||
| denovo_df['file_stem'] = dn_stems |
There was a problem hiding this comment.
Fixed in b798357 — rows where scan parsing returned None are now dropped with dropna() and the column is cast to int before the join.
- Guard _parse_scan/_parse_stem against NaN/None spectra_ref values by coercing to str before applying regex - Drop rows where scan parsing failed and cast scan column to int so the join key is well-typed and matches are not silently lost - Pass n_bootstraps CLI argument through to run_procedure instead of hard-coding 250 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/glissade/preprocessing.py`:
- Around line 40-48: Initialize skiprows before the loop in the preprocessing
logic so it is always defined even if denovo_file is empty or malformed, then
keep using it when calling pd.read_csv. Update the code around run_map and the
loop that parses MTD/PSH lines in preprocessing.py to set a safe default for
skiprows before enumerate(f_in), so the denovo_df read path does not raise a
NameError.
- Around line 32-33: Guard the new file_stem and scan join in preprocessing by
validating that db_df contains filename before using it, and by normalizing both
sides to the same basename consistently. In preprocessing.py, update the logic
around the file_stem derivation and the join against the mzTab scan data so
missing filename values raise a clear error or fall back safely instead of
causing a KeyError or silent row loss. Also add logging in the code path that
builds or merges on file_stem to report unmatched stems when no overlap is
found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f60dde3-2989-462e-ac79-cd71dfde773e
📒 Files selected for processing (2)
src/glissade/glissade.pysrc/glissade/preprocessing.py
- Guard db_df['filename'] access with a clear ValueError if the column is absent, preventing a silent KeyError on non-standard psms.txt files - Initialize skiprows=0 before the mzTab header loop to prevent NameError if the file is empty or malformed - Log any file stems present in only one of the two inputs after the join, making stem-mismatch problems visible Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| denovo_df['file_stem'] = dn_stems | ||
| n_before = len(denovo_df) | ||
| denovo_df = denovo_df.dropna(subset=['scan']) | ||
| denovo_df['scan'] = denovo_df['scan'].astype(int) | ||
| if len(denovo_df) < n_before: | ||
| print(f" Dropped {n_before - len(denovo_df)} Casanovo rows with unparseable spectra_ref") |
There was a problem hiding this comment.
Fixed in 91f097f — rows with an empty file_stem are now dropped alongside rows with unparseable scan numbers, so they no longer appear in the unmatched-stems warning.
| print(f'--- Running FDR control on {len(external_scores)} external peptides using {len(matched_scores)} matched scores ---') | ||
| fdrs, peps, scores = run_procedure(matched_scores, external_scores, external_peps, n_boots=n_bootstraps) |
There was a problem hiding this comment.
Fixed in 91f097f — added explicit ValueError guards before calling run_procedure() when either matched_scores or external_scores is empty, with actionable messages.
- Drop Casanovo rows with empty file_stem (unmatched ms_run entries) alongside rows with unparseable scan numbers, preventing them from polluting the unmatched-stems warning with empty-string entries - Guard run_procedure() against empty matched_scores or external_scores with clear ValueError messages rather than a cryptic crash inside the FDR procedure Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
(file_stem, scan), extracting the file stem from Percolator'sfilenamecolumn and from the mztabMTDheader'sms_run-to-location mapping.x.split('.')approach broke when modification masses contained decimal points (e.g.Q[0.984]), causing flanking residues to be included in the sequence. Fixed by using thex[2:-2]slice directly.--fdrargument: Allows the Percolator FDR threshold used for calibration to be set at the command line (default: 0.01).from preprocessing import ...to relativefrom .preprocessing import ...so the package works correctly when invoked aspython -m glissade.glissade.if __name__ == '__main__'guard and writeglissade_stats.jsonwith matched/external peptide counts.import osto module scope and guardspectra_refparsing against malformed entries.Test plan
--fdr 0.1gives more matched calibration peptides than the default--fdr 0.01glissade_stats.jsonis written with correct counts🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--fdroption to let you control the database FDR threshold from the command line.Bug Fixes
.mztabinputs by skipping entries that can’t be parsed cleanly.Chores