fix: bug in merge result#46
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? |
|
Warning Review limit reached
More reviews will be available in 53 minutes and 48 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThe ChangesMerge Algorithm Results Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 | 4 |
| 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 (2)
onsite/onsitec.py (2)
188-188:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPass
id_fileinto theallcommand's merge call.
merge_algorithm_results()now requiresinput_idparquet, but Line 188 still passes four arguments.onsite all ...will fail withTypeErrorbefore any merged output is written.Suggested fix
- merge_algorithm_results(ascore_out, phosphors_out, lucxor_out, out_file) + merge_algorithm_results(ascore_out, phosphors_out, lucxor_out, out_file, id_file)🤖 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` at line 188, The call to merge_algorithm_results in the "all" command is missing the new required input_idparquet parameter; update the call merge_algorithm_results(ascore_out, phosphors_out, lucxor_out, out_file) to include the id file variable (e.g., id_file) as the first or named input_idparquet argument so it matches the new signature merge_algorithm_results(input_idparquet, ascore_out, phosphors_out, lucxor_out, out_file); locate the call where the "all" command invokes merge_algorithm_results and add the id_file/input_idparquet parameter.
295-304:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse the real
peptide_identification_index, not the join tuple's row index.
_join_psms_by_ref()returns DataFrame indices forai,pi, andli(the merge test assertsliagainstlucxor.index). This loop then filterspeptide_identification_index == ai/pi/liand writesliback out on Line 371. Once a peptide identification has multiple hits, the merge starts reading and updating the wrong rows.Suggested fix
for ai, pi, li in triples: + a_pep_idx = int(ascore_df.iloc[ai]["peptide_identification_index"]) + p_pep_idx = int(phosphors_df.iloc[pi]["peptide_identification_index"]) + l_pep_idx = int(lucxor_df.iloc[li]["peptide_identification_index"]) + a_metas = _get_metas_dict(ascore_df, ai) p_metas = _get_metas_dict(phosphors_df, pi) l_metas = _get_metas_dict(lucxor_df, li) # Get the actual row data from LucXor for base properties for hit_idx in range(3): - a_hit_row = ascore_df[(ascore_df["peptide_identification_index"] == ai) & (ascore_df["hit_index"] == hit_idx)] - p_hit_row = phosphors_df[(phosphors_df["peptide_identification_index"] == pi) & (phosphors_df["hit_index"] == hit_idx)] - l_hit_row = lucxor_df[(lucxor_df["peptide_identification_index"] == li) & (lucxor_df["hit_index"] == hit_idx)] + a_hit_row = ascore_df[(ascore_df["peptide_identification_index"] == a_pep_idx) & (ascore_df["hit_index"] == hit_idx)] + p_hit_row = phosphors_df[(phosphors_df["peptide_identification_index"] == p_pep_idx) & (phosphors_df["hit_index"] == hit_idx)] + l_hit_row = lucxor_df[(lucxor_df["peptide_identification_index"] == l_pep_idx) & (lucxor_df["hit_index"] == hit_idx)] ... - "peptide_identification_index": li, + "peptide_identification_index": l_pep_idx,Also applies to: 371-371
🤖 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 295 - 304, The loop is using the tuple values ai/pi/li (which are DataFrame indices returned by _join_psms_by_ref) to filter rows by the peptide_identification_index column, causing the wrong rows to be read/updated when a peptide_identification has multiple hits; change the three filters to match the DataFrame index instead (e.g., replace ascore_df["peptide_identification_index"] == ai with ascore_df.index == ai, or better use ascore_df.loc[ai] combined with hit_index==hit_idx) for a_hit_row, p_hit_row, and l_hit_row and ensure the subsequent update that writes li back uses the same index-based selection so you update the correct rows.
🤖 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/onsitec.py`:
- Around line 379-384: The code fails when merged_rows is empty because out_df
has no columns and set_index(["peptide_identification_index","hit_index"])
raises KeyError; fix by detecting this zero-merge case right after out_df =
pd.DataFrame(merged_rows) and constructing an out_df that contains the expected
index columns and score columns (or at minimum the peptide_identification_index
and hit_index columns) populated from lucxor_df unique pairs (or set to NaN)
before calling set_index; ensure you reference merged_rows, out_df, full_df,
lucxor_df and use set_index(["peptide_identification_index","hit_index"]) only
after this fallback is in place so the downstream merge/writes produce rows with
missing scores instead of crashing.
---
Outside diff comments:
In `@onsite/onsitec.py`:
- Line 188: The call to merge_algorithm_results in the "all" command is missing
the new required input_idparquet parameter; update the call
merge_algorithm_results(ascore_out, phosphors_out, lucxor_out, out_file) to
include the id file variable (e.g., id_file) as the first or named
input_idparquet argument so it matches the new signature
merge_algorithm_results(input_idparquet, ascore_out, phosphors_out, lucxor_out,
out_file); locate the call where the "all" command invokes
merge_algorithm_results and add the id_file/input_idparquet parameter.
- Around line 295-304: The loop is using the tuple values ai/pi/li (which are
DataFrame indices returned by _join_psms_by_ref) to filter rows by the
peptide_identification_index column, causing the wrong rows to be read/updated
when a peptide_identification has multiple hits; change the three filters to
match the DataFrame index instead (e.g., replace
ascore_df["peptide_identification_index"] == ai with ascore_df.index == ai, or
better use ascore_df.loc[ai] combined with hit_index==hit_idx) for a_hit_row,
p_hit_row, and l_hit_row and ensure the subsequent update that writes li back
uses the same index-based selection so you update the correct rows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| out_df = pd.DataFrame(merged_rows) | ||
| save_dataframes(output_file, out_df, proteins_df, template_df=lucxor_df) | ||
|
|
||
| full_df = lucxor_df.copy() | ||
|
|
||
| out_df = out_df.set_index(["peptide_identification_index", "hit_index"]) | ||
| full_df = full_df.set_index(["peptide_identification_index", "hit_index"]) |
There was a problem hiding this comment.
Handle the zero-merge path before calling set_index.
When no triples survive the join, pd.DataFrame(merged_rows) has no columns, so Line 383 raises KeyError instead of writing an output where every score is marked missing. This is reachable whenever the tools have no common spectrum_reference or every common PSM is skipped for sequence mismatch.
Suggested fix
- out_df = pd.DataFrame(merged_rows)
-
full_df = lucxor_df.copy()
+ out_df = (
+ pd.DataFrame(merged_rows)
+ if merged_rows
+ else full_df.iloc[0:0].copy()
+ )
out_df = out_df.set_index(["peptide_identification_index", "hit_index"])
full_df = full_df.set_index(["peptide_identification_index", "hit_index"])🤖 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 379 - 384, The code fails when merged_rows is
empty because out_df has no columns and
set_index(["peptide_identification_index","hit_index"]) raises KeyError; fix by
detecting this zero-merge case right after out_df = pd.DataFrame(merged_rows)
and constructing an out_df that contains the expected index columns and score
columns (or at minimum the peptide_identification_index and hit_index columns)
populated from lucxor_df unique pairs (or set to NaN) before calling set_index;
ensure you reference merged_rows, out_df, full_df, lucxor_df and use
set_index(["peptide_identification_index","hit_index"]) only after this fallback
is in place so the downstream merge/writes produce rows with missing scores
instead of crashing.
2 participants
Summary by CodeRabbit
Release Notes