fixed bugs for proteomicslfq#49
Merged
Merged
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? |
Contributor
📝 WalkthroughWalkthroughAdds a ChangesModification sites reassigned metric and modeling score threshold
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 | 16 |
| Duplication | 2 |
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/ascore/cli.py`:
- Line 223: The click.echo call on line 223 uses an f-string prefix without any
placeholder expressions inside the string, which is unnecessary and flagged as
unused f-string (F541). Remove the f prefix from the string "\nProcessing
Complete:" in the click.echo call to convert it to a regular string literal,
since there are no variables or expressions to interpolate.
- Around line 207-220: The relocated_count calculation has inconsistent
granularity: the orig_pf_map is built using only the first row from each group
(ID-level baseline with hit_index==0), but the comparison loop iterates through
all rows in result_rows regardless of hit_index. This causes overcounting for
multi-hit identifications. Fix this by ensuring both the baseline construction
and the comparison loop use the same granularity—either filter result_rows to
only include hit_index==0 when comparing against orig_pf_map, or extend
orig_pf_map to be keyed by both peptide_identification_index and hit_index
consistently on both sides.
In `@onsite/idparquet.py`:
- Around line 268-269: The for loop in the _save_psms_parquet function currently
only updates the peptidoform column, but the score-related columns (score,
score_type, and higher_score_better) that are produced by the scoring CLIs are
missing from the updates and not being persisted. Restore the missing column
updates by adding "score", "score_type", and "higher_score_better" to the tuple
in the for loop iteration alongside "peptidoform" so that all four columns are
included in the updates dictionary.
In `@onsite/lucxor/cli.py`:
- Around line 791-795: The issue is that pep_idx from enumerate(all_psms) is
being used to index _best, but these DataFrames are not aligned because _best is
a filtered subset where hit_index equals 0. This causes the code to compare
peptidoforms from mismatched rows, resulting in incorrect relocated_count
tracking. Instead of using pep_idx directly to index _best, you need to find the
corresponding row in _best that matches the current peptide from all_psms by
using the actual data values (such as comparing on a key column like peptidoform
or an identifier that uniquely identifies each PSM), then retrieve the original
peptidoform from that matched row before comparing with the current peptidoform
value.
In `@onsite/onsitec.py`:
- Around line 411-425: The merge_algorithm_results function now requires the
input_idparquet parameter (used in the function body to load input dataframes),
but the call site in the all() function is still passing only four positional
arguments instead of five. Update the merge_algorithm_results invocation in the
all() function to include input_idparquet as an additional argument so that it
matches the function signature and provides the required parameter that the
function body depends on.
- Around line 415-424: The first loop iterating through input_psms_df stores
peptidoforms in input_pf_map using only peptide_identification_index as the key,
which causes overwrites when multiple rows share the same identification index.
This means the comparison at line 423 uses an arbitrary peptidoform value
instead of a consistent one. Modify the first loop to only store peptidoforms
when hit_index equals 0 for each peptide_identification_index, ensuring that
only the first hit's peptidoform is stored and compared against in the
relocation count logic.
In `@onsite/phosphors/cli.py`:
- Line 273: The string literal in the click.echo call contains an unnecessary
f-string prefix since there are no variable interpolations within the string.
Remove the f prefix from the "\nProcessing Complete:" string so it becomes a
regular string literal instead of an f-string.
- Around line 256-269: The relocated_count calculation has a mismatch in scope:
the baseline orig_pf_map stores one peptidoform per peptide_identification_index
(ID-level), but the comparison loop iterates through all result_rows, causing
overcounting when an ID has multiple hits. Filter the result_rows loop to only
count relocations for rows that correspond to the primary hit (such as where
hit_index equals 0) to ensure the row-level comparison aligns with the ID-level
baseline stored in orig_pf_map.
In `@pyproject.toml`:
- Line 3: The version in pyproject.toml is being bumped to 0.0.4, but the CLI
version option in onsite/onsitec.py still references the old version 0.0.3.
Update the `@click.version_option` decorator's version parameter in the onsitec.py
file to match the new version 0.0.4 specified in pyproject.toml, ensuring that
the `onsite --version` command reports the correct version.
🪄 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: d3a14f94-17fc-4f16-aef1-d8a57479b976
📒 Files selected for processing (7)
onsite/ascore/cli.pyonsite/idparquet.pyonsite/lucxor/cli.pyonsite/onsitec.pyonsite/phosphors/cli.pypyproject.tomltests/test_output_validation.py
Contributor
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 (2)
onsite/onsitec.py (2)
182-200:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck the LucXor exit code before merging.
PyLuciPHOr2.run()returns a status code, butalldrops it here. If LucXor returns non-zero, the merge still runs againstlucxor_out.🐛 Proposed fix
lucxor_setup_logging(debug, None, lucxor_out) tool = PyLuciPHOr2() - tool.run( + exit_code = tool.run( input_spectrum=in_file, input_id=id_file, output=lucxor_out, fragment_method=fragment_method, fragment_mass_tolerance=fragment_mass_tolerance, @@ modeling_score_threshold=modeling_score_threshold, scoring_threshold=0.0, min_num_psms_model=50, threads=threads, rt_tolerance=0.01, debug=debug, disable_split_by_charge=False, ) + if exit_code != 0: + raise RuntimeError(f"LucXor failed with exit code {exit_code}")🤖 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/onsitec.py` around lines 182 - 200, The return value from tool.run() in the PyLuciPHOr2 class is not being checked for error status codes. Capture the return value from the tool.run() method call and verify it returns a successful status (typically zero). If the return value indicates an error (non-zero), log an error message and exit the process before attempting any subsequent operations on lucxor_out, ensuring that failed LucXor runs do not proceed to the merge step.
431-436:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCanonicalize peptidoforms before comparing merged reassignments.
The final metric compares raw strings, so case/notation-only changes between the input idParquet and LucXor output can inflate
Modification sites reassigned.🐛 Proposed fix
+ def _canonical_peptidoform(value): + if value is None or pd.isna(value): + return "" + text = str(value) + return unimod_to_pyopenms_notation(text).upper() + for _, row in out_df.iterrows(): pep_idx = row.get("peptide_identification_index") - out_pf = str(row.get("peptidoform", "")) - orig_pf = input_pf_map.get(pep_idx, "") + out_pf = _canonical_peptidoform(row.get("peptidoform", "")) + orig_pf = _canonical_peptidoform(input_pf_map.get(pep_idx, "")) if orig_pf and orig_pf != out_pf: relocated_count += 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/onsitec.py` around lines 431 - 436, The comparison of peptidoforms in the loop that iterates over out_df.iterrows() is performing a raw string comparison between orig_pf and out_pf without canonicalizing them first, which causes case or notation-only differences to be treated as actual changes and inflates the relocated_count metric. Canonicalize both the orig_pf and out_pf variables before comparing them in the if statement to ensure only genuine peptidoform differences are counted.
♻️ Duplicate comments (1)
onsite/lucxor/cli.py (1)
800-804:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCanonicalize both peptidoforms before counting a reassignment.
peptidoformis uppercased on Line 790, butorig_peptidoformis compared raw here. Inputs usingPhospho/mixed-case notation can be counted as reassigned even when the site did not move.🐛 Proposed fix
# Track how many peptides had their modification positions changed if _in_rec is not None: - orig_peptidoform = _in_rec["peptidoform"] + orig_peptidoform = str(_in_rec["peptidoform"]).upper() if orig_peptidoform and orig_peptidoform != peptidoform: relocated_count += 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/cli.py` around lines 800 - 804, The comparison at lines 803-804 between `orig_peptidoform` and `peptidoform` is inconsistent because `peptidoform` has been canonicalized (uppercased) on line 790, but `orig_peptidoform` is compared in its raw form from `_in_rec["peptidoform"]`. This causes false positives when inputs use mixed-case notation like Phospho. Apply the same canonicalization logic to `orig_peptidoform` before the comparison so that both values are in canonical form when checking if they differ, ensuring only actual site reassignments are counted.
🤖 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 875-879: The label "Relocated" in the LucXor summary output is
inconsistent with the standardized label "Modification sites reassigned" used in
other localization summaries. Change the print statement that outputs
relocated_count to use "Modification sites reassigned" instead of "Relocated" to
maintain consistency across all localization summary outputs.
---
Outside diff comments:
In `@onsite/onsitec.py`:
- Around line 182-200: The return value from tool.run() in the PyLuciPHOr2 class
is not being checked for error status codes. Capture the return value from the
tool.run() method call and verify it returns a successful status (typically
zero). If the return value indicates an error (non-zero), log an error message
and exit the process before attempting any subsequent operations on lucxor_out,
ensuring that failed LucXor runs do not proceed to the merge step.
- Around line 431-436: The comparison of peptidoforms in the loop that iterates
over out_df.iterrows() is performing a raw string comparison between orig_pf and
out_pf without canonicalizing them first, which causes case or notation-only
differences to be treated as actual changes and inflates the relocated_count
metric. Canonicalize both the orig_pf and out_pf variables before comparing them
in the if statement to ensure only genuine peptidoform differences are counted.
---
Duplicate comments:
In `@onsite/lucxor/cli.py`:
- Around line 800-804: The comparison at lines 803-804 between
`orig_peptidoform` and `peptidoform` is inconsistent because `peptidoform` has
been canonicalized (uppercased) on line 790, but `orig_peptidoform` is compared
in its raw form from `_in_rec["peptidoform"]`. This causes false positives when
inputs use mixed-case notation like Phospho. Apply the same canonicalization
logic to `orig_peptidoform` before the comparison so that both values are in
canonical form when checking if they differ, ensuring only actual site
reassignments are counted.
🪄 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: 488d9277-67c8-420a-a096-1e4c6ae43e95
📒 Files selected for processing (3)
onsite/ascore/cli.pyonsite/lucxor/cli.pyonsite/onsitec.py
🚧 Files skipped from review as they are similar to previous changes (1)
- onsite/ascore/cli.py
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.
Summary by CodeRabbit
New Features
Improvements
modeling_score_thresholdoption (default0.95) for the “all” command and single-run LucXor execution.peptidoformfield while leaving other existing values unchanged.Tests
--modeling-score-threshold 0.3.Chores
0.0.4.