fixed bugs and clean up#47
Conversation
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? |
📝 WalkthroughWalkthroughThe CLI now loads spectra and constructs/scores PSMs inside load_input_files(config), stashing mz/rt/scan and reference metadata on PSMs. run() calls the refactored loader (exits on no PSMs/spectra). Output formatting reads stashed metadata and uses a regex-based fallback for base sequences; sequence conversion preserves non-target mods. ChangesPSM Early Construction and Output Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -10 |
| Duplication | 0 |
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
onsite/lucxor/cli.py (1)
424-479:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCarry the source row identity instead of rejoining by
pep_idx.
all_psmsis built from everypsms_dfrow, but the output path later indexes intoself._psms_df_template[self._psms_df_template["hit_index"] == 0]usingpep_idx. As soon as the input containshit_index != 0rows, this reattaches the wrongpsm_metavaluesand emits syntheticpeptide_identification_indexvalues that no longer match the source identification. Stash the original row/index metadata on thePSMduring construction and reuse that directly when writing results.Also applies to: 797-845
🤖 Prompt for 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. In `@onsite/lucxor/cli.py` around lines 424 - 479, The loop building all_psms currently loses the original psms_df row identity and later reindexes by pep_idx causing mismatched psm_metavalues; when constructing each PSM (in the block creating `psm`), attach the source row identifier and/or the DataFrame index (e.g., set psm._source_row_index = scan_num or psm._source_df_index = row.name and/or stash the entire row via psm._source_row) so the writer can look up metadata directly from the original row instead of rejoining by `pep_idx`; apply the same change in the other similar block mentioned (lines ~797-845) and update downstream writer logic to use these new psm._source_* fields when building psm_metavalues and peptide_identification_index.
🤖 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 `@onsite/lucxor/cli.py`:
- Around line 418-420: The code is forcing PEP scoring by overwriting
self.score_type and self.higher_score_better in load_input_files() and seeding
every PeptideHit from posterior_error_probability; revert/remove the hard-coded
assignments to self.score_type/self.higher_score_better and instead read the
dataset's declared score_type/higher_score_better per input (use the row's
score_type and additional_scores fields) and seed each PeptideHit with the
appropriate score field (use posterior_error_probability only when score_type ==
"Posterior Error Probability", otherwise use the primary score or the matching
key in additional_scores); update load_input_files() and any code that
constructs PeptideHit to respect per-row score_type/additional_scores so run()'s
E-value/raw-score branches remain reachable.
---
Outside diff comments:
In `@onsite/lucxor/cli.py`:
- Around line 424-479: The loop building all_psms currently loses the original
psms_df row identity and later reindexes by pep_idx causing mismatched
psm_metavalues; when constructing each PSM (in the block creating `psm`), attach
the source row identifier and/or the DataFrame index (e.g., set
psm._source_row_index = scan_num or psm._source_df_index = row.name and/or stash
the entire row via psm._source_row) so the writer can look up metadata directly
from the original row instead of rejoining by `pep_idx`; apply the same change
in the other similar block mentioned (lines ~797-845) and update downstream
writer logic to use these new psm._source_* fields when building psm_metavalues
and peptide_identification_index.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| # PEP converted 1-PEP — set early so get_psm_score uses it inline | ||
| self.score_type = "Posterior Error Probability" | ||
| self.higher_score_better = True |
There was a problem hiding this comment.
Don't force every dataset onto PEP scoring.
load_input_files() now hard-codes self.score_type/self.higher_score_better to PEP and seeds every PeptideHit from posterior_error_probability, even though each row still carries score_type and additional_scores. That makes the E-value/raw-score branches in run() unreachable and can train on the wrong PSM subset whenever the input's primary score is something like expect, SpecEValue, or xcorr.
Also applies to: 432-433, 466-470
🤖 Prompt for 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.
In `@onsite/lucxor/cli.py` around lines 418 - 420, The code is forcing PEP scoring
by overwriting self.score_type and self.higher_score_better in
load_input_files() and seeding every PeptideHit from
posterior_error_probability; revert/remove the hard-coded assignments to
self.score_type/self.higher_score_better and instead read the dataset's declared
score_type/higher_score_better per input (use the row's score_type and
additional_scores fields) and seed each PeptideHit with the appropriate score
field (use posterior_error_probability only when score_type == "Posterior Error
Probability", otherwise use the primary score or the matching key in
additional_scores); update load_input_files() and any code that constructs
PeptideHit to respect per-row score_type/additional_scores so run()'s
E-value/raw-score branches remain reachable.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
onsite/lucxor/psm.py (1)
1673-1699:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNon-target mod mapping is still incorrect for several lowercase residues.
At Line 1694, lookup uses raw index
i, butself.non_target_modsis residue-position based. Also, Lines 1674-1689 consumes/t/y/m/w/f/ybefore checkingself.non_target_mods, so non-target mods on those residues are serialized as Phospho/Oxidation instead of their true mod names.Suggested fix
try: if not sequence: return sequence result = "" i = 0 + residue_pos = 0 while i < len(sequence): - if sequence[i].islower() and sequence[i].upper() in ["S", "T", "Y"]: - # Convert lowercase to uppercase and add (Phospho) - result += sequence[i].upper() + "(Phospho)" - elif sequence[i] == "a": + ch = sequence[i] + + if ch in "[]()": + result += ch + i += 1 + continue + + if ch == "a": # Lowercase 'a' = PhosphoDecoy on Alanine; emit the standard # annotation so an A-win is serializable and countable # downstream (see bigbio/onsite#40). result += "A(PhosphoDecoy)" - elif sequence[i].islower() and sequence[i].upper() in [ - "M", - "W", - "F", - "Y", - ]: - # Convert lowercase to uppercase and add (Oxidation) - result += sequence[i].upper() + "(Oxidation)" - elif sequence[i].islower(): + residue_pos += 1 + elif ch.islower(): # Non-target modification (e.g. Carbamidomethyl on C) — # look up the modification name from self.non_target_mods. - aa_upper = sequence[i].upper() - mod_name = self.non_target_mods.get(i) + aa_upper = ch.upper() + mod_name = self.non_target_mods.get(residue_pos) if mod_name: result += f"{aa_upper}({mod_name})" + elif aa_upper in ["S", "T", "Y"]: + result += aa_upper + "(Phospho)" + elif aa_upper in ["M", "W", "F", "Y"]: + result += aa_upper + "(Oxidation)" else: # Fallback: emit the bare upper-case letter. result += aa_upper + residue_pos += 1 else: - result += sequence[i] + result += ch + if ch.isalpha(): + residue_pos += 1 i += 1🤖 Prompt for 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. In `@onsite/lucxor/psm.py` around lines 1673 - 1699, The code currently checks residue-type branches (Phospho/Oxidation) before consulting self.non_target_mods and also looks up non-target mods with the raw loop index i, causing wrong serialization; fix by computing the residue-position key (e.g., residue_pos = count of alphabetic residues up to i) and consult self.non_target_mods at the top of the loop: if a mod_name exists for that residue_pos, append aa_upper+"("+mod_name+")" to result and skip other branches; otherwise continue with the existing Phospho/Oxidation/PhosphoDecoy/fallback logic using sequence, i, result, and aa_upper as before.
🤖 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.
Outside diff comments:
In `@onsite/lucxor/psm.py`:
- Around line 1673-1699: The code currently checks residue-type branches
(Phospho/Oxidation) before consulting self.non_target_mods and also looks up
non-target mods with the raw loop index i, causing wrong serialization; fix by
computing the residue-position key (e.g., residue_pos = count of alphabetic
residues up to i) and consult self.non_target_mods at the top of the loop: if a
mod_name exists for that residue_pos, append aa_upper+"("+mod_name+")" to result
and skip other branches; otherwise continue with the existing
Phospho/Oxidation/PhosphoDecoy/fallback logic using sequence, i, result, and
aa_upper as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c06bdac9-caab-45c4-bbf3-36cefdd37293
📒 Files selected for processing (2)
onsite/lucxor/cli.pyonsite/lucxor/psm.py
🚧 Files skipped from review as they are similar to previous changes (1)
- onsite/lucxor/cli.py
daichengxin
changed the title
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor