Format-agnostic identification I/O: idXML / mzIdentML / idParquet#52
Merged
Conversation
…to psms_df Adds onsite/id_io.py (Phase 1 load side): - detect_format(): idparquet/idxml/mzid by extension or directory - load_identifications(): 4-tuple dispatch to idparquet.load_dataframes or pyOpenMS IdXMLFile/MzIdentMLFile then _peptide_ids_to_psms_df() - _peptide_ids_to_psms_df(): 29-col schema-compatible DataFrame with int32 scan/charge/hit_index, psm_metavalues as numpy ndarray of dicts, and a score fallback for mzid round-trip that drops the primary score into a named metavalue Adds tests/test_id_io.py: 25 tests covering all three formats and edge cases.
… value_type + empty-df dtypes
- protein_accessions: populate from hit.getPeptideEvidences() as numpy object
array of {accession, aa_before, aa_after, start, end} dicts matching fixture schema
- modifications: populate via peptidoform_to_modifications(peptidoform) per row
- psm_metavalues value_type: derive from Python type (float→"double", int→"int",
else "string") matching fixture convention; add _metavalue_type_str() helper
- _empty_psms_df(): enforce int32/float64/bool dtypes via _PSM_DTYPE_MAP so
empty and non-empty DataFrames are type-consistent
- detect_format: coerce path=str(path) so pathlib.Path works
- _NON_SCORE_KEYS: hoist to module level (was rebuilt per hit); expand to include
AScore_site_scores, num_matched_peptides, protein_references, isotope_error
- _SCAN_RE: tighten to \bscan=(\d+) to avoid false-matching basescan=123
- tests: add TestDetectFormatPathlib (4), TestProteinAccessions (2),
TestMetavalueType (3), TestModificationsPopulated (2); 36/36 pass
…writer - Add save_identifications() to id_io.py dispatching on detect_format(): idParquet (from-scratch or schema-copy), idXML, mzIdentML - Add _psms_df_to_peptide_ids() converting psms DataFrame to pyOpenMS objects - Add save_psms_from_scratch() to idparquet.py for schema-free idParquet write - Fix latent bug in _copy_or_create_parquet() where src was computed without None-guard on source_idparquet - Add 11 Phase 2 tests (4 classes): idParquet/idXML/mzid roundtrip + scratch write All 47 tests pass
…metavalue round-trip tests BUG 1: save_psms_from_scratch now fills NaN with -1 sentinel before casting int32 columns (precursor_charge, scan, hit_index, peptide_identification_index). Previously the silent try/except left NaN-containing columns as float64, producing schema-mismatched parquet output. BUG 2: _psms_df_to_peptide_ids now uses pd.isna() throughout for scalar values that could be None, NaN, or pd.NA: score_type, higher_score_better (which raised "boolean value of NA is ambiguous"), rt_val, mz_val, charge, and score_val. GAP 3: test_phosphodecoy_does_not_raise rewritten with a real ALS(Phospho)A(PhosphoDecoy)K peptidoform (UNIMOD:21 + UNIMOD:99913). Empirically confirmed: the PhosphoDecoy strip in save_identifications is REACHABLE (strips PhosphoDecoy from SearchParams.variable_modifications) but the hit sequence survives, and PhosphoDecoy round-trips correctly via the mzid save→load path. Test now asserts "PhosphoDecoy" appears in unimod_to_pyopenms_notation(reloaded_peptidoform). GAP 4: Added test_mzid_score_preserved and test_mzid_metavalue_survives to TestSaveLoadMzidRoundtrip for score precision (< 1e-6) and AScore metavalue survival through mzid round-trip. Minor: Added test_from_scratch_peptidoform_preserved and test_from_scratch_int32_dtypes to TestSaveIdparquetFromScratch. Added TODO(phase3) comments for search_params.parquet stub and template_df parameter wiring.
…/onsitec (multi-format I/O) Phase 3 of unified identification I/O: replace direct load_dataframes/save_dataframes calls at user-facing I/O boundaries with load_identifications/save_identifications from onsite.id_io, enabling transparent multi-format support (.idparquet, .idxml, .mzid). Internal per-tool intermediate files (ascore/phosphors/lucxor .idparquet) continue to use idparquet.load_dataframes/save_dataframes directly. Also adds TestRoundTripFormats to tests/test_id_io.py verifying row-count preservation across all three output formats via the fixture idParquet.
…rmat round-trip - Remove `save_dataframes as _save_df` from phosphors/cli.py (replaced by _io_save) - Remove `load_dataframes`, `save_dataframes` from ascore/cli.py (replaced by load/save_identifications) - Remove `save_dataframes`, `load_dataframes` from lucxor/cli.py (replaced by load/save_identifications) - Add _inject_ascore_metavalue helper to TestRoundTripFormats - Add test_round_trip_idxml_score_key_survives: AScore_site_scores survives idXML round-trip - Add test_round_trip_mzid_score_key_survives: AScore_site_scores survives mzid round-trip - Add test_round_trip_idparquet_score_key_survives: AScore_site_scores survives idparquet round-trip
…ions onto idParquet main Cherry-pick of the perf work onto main's idParquet architecture. Core optimizations apply cleanly (ascore.py numpy matching, phosphors.py binomial cache + theo cache, lucxor/psm.py density hoist). The ascore/cli.py shared-AScore -instance tweak was dropped: main refactored that CLI to DataFrame groups (process_psm_group), so it no longer applies; the primary AScore win lives in ascore.py and is preserved.
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
📝 WalkthroughWalkthroughIntroduces ChangesUnified Identification I/O Layer
Algorithm Performance Optimizations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|---|---|
| Complexity | 9 medium |
🟢 Metrics 170 complexity · 2 duplication
Metric Results Complexity 170 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.
Algorithm Comparison Test ResultsClick to expand test results |
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
onsite/phosphors/cli.py (1)
137-147:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize the debug logger before loading inputs.
With
--debug, a failure fromload_spectra()or the new_io_load()path leaves localloggerundefined, so the outer exception handler can raiseUnboundLocalErrorinstead of reporting the real input error.🐛 Proposed fix
try: # Initialize processing pipeline + log_file = f"{out_file}.debug.log" + logger = log_debug(log_file, debug) exp = load_spectra(in_file) psms_df, proteins_df = load_identifications(id_file) # Build scan number to spectrum mapping for efficient lookup lookup = build_scan_to_spectrum_map(exp) # Initialize debug log (only when --debug) - log_file = f"{out_file}.debug.log" - logger = log_debug(log_file, debug) if debug:🤖 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/phosphors/cli.py` around lines 137 - 147, The debug logger initialization using log_debug() happens after load_spectra() and load_identifications() are called, so if either of these functions fails when --debug is enabled, the logger variable will be undefined and cause an UnboundLocalError in the exception handler instead of reporting the actual input error. Move the logger initialization using log_debug() to the beginning of the try block, before calling load_spectra() and load_identifications(), so the logger is always available for exception handling.
🧹 Nitpick comments (4)
onsite/phosphors/phosphors.py (1)
136-140: 💤 Low valueOptional: Optimize FIFO eviction to avoid full key list allocation.
The current eviction creates a full list of all cache keys (potentially 200,000 elements) just to delete 1-2 oldest entries. While functionally correct, using
next(iter(_binomial_tail_cache))would be more efficient.⚡ Proposed optimization
def _store_binomial_tail(key, value: float) -> float: """Store a computed binomial-tail value under its (k, n, p) key (bounded FIFO). Returns the value so callers can ``return _store_binomial_tail(...)``.""" if len(_binomial_tail_cache) >= BINOMIAL_TAIL_CACHE_SIZE: - for old_key in list(_binomial_tail_cache.keys())[ - : len(_binomial_tail_cache) - BINOMIAL_TAIL_CACHE_SIZE + 1 - ]: + num_to_delete = len(_binomial_tail_cache) - BINOMIAL_TAIL_CACHE_SIZE + 1 + for _ in range(num_to_delete): + old_key = next(iter(_binomial_tail_cache)) del _binomial_tail_cache[old_key] _binomial_tail_cache[key] = value return value🤖 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/phosphors/phosphors.py` around lines 136 - 140, The cache eviction logic in the _binomial_tail_cache cleanup block creates a full list of all dictionary keys just to delete one or two oldest entries, which is inefficient for large caches. Replace the slice-based approach with a loop that repeatedly uses next(iter(_binomial_tail_cache)) to get and delete the oldest (first) key from the dictionary in each iteration, since Python dictionaries maintain insertion order. This avoids allocating a potentially large list and achieves the same FIFO eviction behavior with better performance.onsite/lucxor/cli.py (1)
52-63: ⚡ Quick winUpdate LucXor help text for format-agnostic IDs.
The command now accepts and writes idParquet, idXML, and mzIdentML through
onsite.id_io, but the help still tells users both paths are idParquet-only.♻️ Proposed fix
required=True, type=click.Path(exists=True), - help="Input identification file (idparquet directory)" + help="Input identification file (.idparquet, .idXML, .mzid, or .mzIdentML)" ) @@ required=True, type=click.Path(), - help="Output file (idparquet directory)" + help="Output identification file (.idparquet, .idXML, .mzid, or .mzIdentML)" )🤖 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 52 - 63, The help text for both the input and output options in the click decorators is outdated and only mentions idParquet format support. Update the help parameter in the input option decorator to indicate it accepts multiple ID formats (idParquet, idXML, and mzIdentML) instead of just idparquet directory, and similarly update the help parameter in the output option decorator to reflect the same format-agnostic capabilities since the command now supports these multiple formats through onsite.id_io.tests/test_id_io.py (1)
645-776: ⚡ Quick winAdd save→load coverage for decoy and protein evidence preservation.
The mzIdentML/idXML round-trip tests currently cover rows, peptidoforms, scores, and metavalues, but not
is_decoyorprotein_accessions; those are high-value schema fields for the new conversion layer.🧪 Proposed regression coverage
class TestSaveLoadMzidRoundtrip: @@ def test_mzid_metavalue_survives(self, tmp_path): """A psm_metavalue injected before save must appear in the reloaded row.""" @@ assert "AScore_site_scores" in names_rt, ( f"AScore_site_scores not found in reloaded metavalues: {names_rt}" ) + + def test_mzid_decoy_and_protein_accessions_survive(self, tmp_path): + from onsite.id_io import save_identifications + import pyopenms as oms + + prot, pep_list = _build_pep_list_with_protein() + path_in = str(tmp_path / "source.idXML") + oms.IdXMLFile().store(path_in, prot, pep_list) + psms_df, proteins_df, *_ = load_identifications(path_in) + + path_out = str(tmp_path / "out.mzid") + save_identifications(path_out, psms_df, proteins_df) + psms2, *_ = load_identifications(path_out) + + assert psms2["protein_accessions"].iloc[0][0]["accession"] == "PROT_A"🤖 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 `@tests/test_id_io.py` around lines 645 - 776, Add two new test methods to the TestSaveLoadMzidRoundtrip class following the same pattern as the existing round-trip tests (test_row_count_preserved, test_peptidoform_preserved, test_mzid_score_preserved, etc.). First, create a test_is_decoy_preserved method that loads identifications from idXML, saves to mzid, reloads the data, and asserts that the is_decoy column values are identical before and after the round-trip. Second, create a test_protein_accessions_preserved method that similarly verifies the protein_accessions field values are preserved through the save and load cycle. Both tests should use the same pattern: call _build_pep_list(), store to idXML using pyopenms, load with load_identifications, save with save_identifications to mzid format, reload with load_identifications, and assert the field values match between original and reloaded dataframes.onsite/ascore/ascore.py (1)
443-443: 💤 Low valueConsider using
min()for clarity.The condition
i if size > i else sizeis equivalent tomin(i, size)and reads more directly.♻️ Suggested simplification
- k = i if size > i else size + k = min(i, size)🤖 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/ascore/ascore.py` at line 443, The assignment to variable k uses a ternary conditional operator to get the smaller of two values. Replace the ternary expression `i if size > i else size` with the built-in min() function by assigning `k = min(i, size)` instead, which is more concise and clearly expresses the intent to get the minimum value between i and size.
🤖 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`:
- Around line 114-119: The debug logger is initialized after calling
load_spectra() and load_identifications(), which means if either function raises
an exception during debug mode, the except block will reference an undefined
logger variable and mask the original error. Move the log_debug call that
assigns to the logger variable to occur before the calls to load_spectra() and
load_identifications() so that the logger is available in the except block
regardless of where the exception originates.
In `@onsite/id_io.py`:
- Around line 127-128: The detect_format() function cannot recognize an
idParquet directory format when the path does not yet exist, preventing new
directory-style outputs from being created properly. Before calling
detect_format(path), add logic to handle the case where the directory does not
exist yet. Either modify detect_format() to recognize idParquet format from the
path string itself even when the directory hasn't been created, or add a check
that allows the format detection to infer the intended format for new
directories so that the suffix-normalization path in the scratch writer becomes
reachable for new idParquet outputs.
- Around line 318-334: The fallback score retrieval logic in the nested loop
starting after the check "if score == 0.0" is too permissive and accepts any
numeric metadata value that is not in _NON_SCORE_KEYS, which can corrupt the
score with unrelated numeric data like counts or FLR values. Instead of
iterating through all meta_by_name items and accepting any numeric value,
replace this loop with logic that only accepts score values from an explicit
whitelist of known score key names. Keep the initial attempt to recover the
score from the specific score_type key, but remove or refactor the arbitrary
fallback loop to only consider values from predefined score key candidates.
- Around line 474-512: The _psms_df_to_peptide_ids() function is not restoring
the target_decoy metavalue and PeptideEvidence objects when reconstructing
PeptideHit objects from the DataFrame, causing is_decoy and protein_accessions
data to be lost during idXML/mzIdentML save-load cycles. After setting the score
on the hit object (following hit.setScore), retrieve the is_decoy column value
from the current row and set it as the target_decoy metavalue on the hit, then
retrieve the protein_accessions array from the row and create PeptideEvidence
objects for each protein accession and attach them to the hit before appending
it to the hits list.
In `@onsite/idparquet.py`:
- Around line 402-410: The code silently converts NaN values to True when
casting boolean columns like is_decoy or higher_score_better to bool dtype using
astype(bool), because NaN is treated as True. Modify the casting logic in the
else block after the check for _int32_dtype_cols to identify when the target
dtype is bool and fill missing values with False before calling astype(dtype),
similar to how _int32_dtype_cols columns fill NaN with -1 before casting. This
ensures boolean columns don't have their missing values silently converted to
True during the dtype conversion.
In `@onsite/lucxor/cli.py`:
- Around line 420-435: The code unconditionally uses posterior_error_probability
to score each PeptideHit via the hit.setScore() method without checking if it's
NaN, which occurs for idXML/mzIdentML conversions. Instead, check if the
posterior_error_probability value is NaN, and if so, use the imported PSM score
from the score column. Update both the hit.setScore() call and the
hit.setMetaValue() call to conditionally use either the
posterior_error_probability when it's available, or fall back to the score
column value when it's NaN.
---
Outside diff comments:
In `@onsite/phosphors/cli.py`:
- Around line 137-147: The debug logger initialization using log_debug() happens
after load_spectra() and load_identifications() are called, so if either of
these functions fails when --debug is enabled, the logger variable will be
undefined and cause an UnboundLocalError in the exception handler instead of
reporting the actual input error. Move the logger initialization using
log_debug() to the beginning of the try block, before calling load_spectra() and
load_identifications(), so the logger is always available for exception
handling.
---
Nitpick comments:
In `@onsite/ascore/ascore.py`:
- Line 443: The assignment to variable k uses a ternary conditional operator to
get the smaller of two values. Replace the ternary expression `i if size > i
else size` with the built-in min() function by assigning `k = min(i, size)`
instead, which is more concise and clearly expresses the intent to get the
minimum value between i and size.
In `@onsite/lucxor/cli.py`:
- Around line 52-63: The help text for both the input and output options in the
click decorators is outdated and only mentions idParquet format support. Update
the help parameter in the input option decorator to indicate it accepts multiple
ID formats (idParquet, idXML, and mzIdentML) instead of just idparquet
directory, and similarly update the help parameter in the output option
decorator to reflect the same format-agnostic capabilities since the command now
supports these multiple formats through onsite.id_io.
In `@onsite/phosphors/phosphors.py`:
- Around line 136-140: The cache eviction logic in the _binomial_tail_cache
cleanup block creates a full list of all dictionary keys just to delete one or
two oldest entries, which is inefficient for large caches. Replace the
slice-based approach with a loop that repeatedly uses
next(iter(_binomial_tail_cache)) to get and delete the oldest (first) key from
the dictionary in each iteration, since Python dictionaries maintain insertion
order. This avoids allocating a potentially large list and achieves the same
FIFO eviction behavior with better performance.
In `@tests/test_id_io.py`:
- Around line 645-776: Add two new test methods to the TestSaveLoadMzidRoundtrip
class following the same pattern as the existing round-trip tests
(test_row_count_preserved, test_peptidoform_preserved,
test_mzid_score_preserved, etc.). First, create a test_is_decoy_preserved method
that loads identifications from idXML, saves to mzid, reloads the data, and
asserts that the is_decoy column values are identical before and after the
round-trip. Second, create a test_protein_accessions_preserved method that
similarly verifies the protein_accessions field values are preserved through the
save and load cycle. Both tests should use the same pattern: call
_build_pep_list(), store to idXML using pyopenms, load with
load_identifications, save with save_identifications to mzid format, reload with
load_identifications, and assert the field values match between original and
reloaded dataframes.
🪄 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: f3cb964b-2e42-4882-bc56-3802e7f64954
📒 Files selected for processing (10)
onsite/ascore/ascore.pyonsite/ascore/cli.pyonsite/id_io.pyonsite/idparquet.pyonsite/lucxor/cli.pyonsite/lucxor/psm.pyonsite/onsitec.pyonsite/phosphors/cli.pyonsite/phosphors/phosphors.pytests/test_id_io.py
…s_decoy/protein restore, score-recovery, extensionless idparquet, debug logger order #6 (CRITICAL) lucxor/cli.py: when posterior_error_probability is NaN for all rows, fall back to df's score_type/higher_score_better and use row['score'] instead of NaN for hit.setScore(); skip 'Posterior Error Probability' metavalue. #5 (Major) idparquet.py save_psms_from_scratch: bool columns (is_decoy, higher_score_better) now filled with False before astype(bool) to prevent NaN→True coercion. Uses map-based approach to avoid pandas FutureWarning. #4 (Major) id_io.py _psms_df_to_peptide_ids: restore is_decoy as target_decoy metavalue and rebuild PeptideEvidence objects from protein_accessions dicts on the save path. Also register all referenced protein accessions as ProteinHit entries in ProteinIdentification (required by idXML format). #3 (Major) id_io.py score-recovery: removed the fallback loop scanning arbitrary numeric metavalues. Only the score_type-named metavalue lookup is kept. Save path now explicitly stores score under score_type name as a metavalue to enable recovery after mzIdentML round-trip (where score_type is renamed). Updated two pre-existing mzid score tests accordingly. #2 (Major) id_io.py save_identifications: wrap detect_format() in try/except; on ValueError with no file extension, treat as idparquet. load side unchanged. #1 (Major) ascore/cli.py: moved logger = log_debug(...) above load_spectra() and load_identifications() calls so the except block can reference logger when either load raises. Tests: 82 passed (test_id_io.py +7 new tests, test_lucxor/ascore/phosphors all green)
Member
Author
|
Addressed all 6 review findings in d1dadd7 (+5 regression tests; full suite 177 passing):
New tests cover each path (these were previously only exercised by end-to-end tests that skip without a local mzML). |
…lidation Fix/phosphors fragment charge validation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
onsite/id_io.py (1)
292-296:⚠️ Potential issue | 🟠 MajorHandle bytes returned by pyOpenMS
getAABefore()/getAAAfter()to prevent data corruption.The codebase already demonstrates defensive bytes handling at line 306 using
k.decode() if isinstance(k, bytes), but lines 293-294 lack this protection. If pyOpenMS returns bytes (likely, given tests usesetAABefore(b"K")), thenstr(b'K')produces"b'K'"instead of"K", corrupting the flanking residue data on save/load round-trip.Suggested fix
pa_list.append({ "accession": ev.getProteinAccession(), - "aa_before": str(ev.getAABefore()), - "aa_after": str(ev.getAAAfter()), + "aa_before": ev.getAABefore().decode() if isinstance(ev.getAABefore(), bytes) else ev.getAABefore(), + "aa_after": ev.getAAAfter().decode() if isinstance(ev.getAAAfter(), bytes) else ev.getAAAfter(), "start": ev.getStart(), "end": ev.getEnd(), })🤖 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/id_io.py` around lines 292 - 296, The aa_before and aa_after fields on lines 293-294 do not defensively handle bytes returned by getAABefore() and getAAAfter() methods. Apply the same bytes-handling pattern already demonstrated at line 306 by checking if the return value is bytes using isinstance() and calling decode() if needed, otherwise use the value as-is. Replace the str() calls around getAABefore() and getAAAfter() with conditional expressions that properly decode bytes to strings.onsite/lucxor/cli.py (1)
881-883:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
protein_accessionsis unconditionally cleared, discarding any existing protein mappings.The output DataFrame sets
protein_accessionsto an empty array, which loses protein-to-peptide associations that were present in the input. If downstream processes rely on protein accessions (e.g., for protein-level reporting or filtering), they will receive no data.Consider copying the original
protein_accessionsfrom the input DataFrame row, similar to howpsm_metavaluesis preserved:Suggested approach
+ # Preserve protein_accessions from input if available + _prot_acc = np.array([], dtype=object) + if _in_rec is not None and "_protein_accessions" in _input_by_ref.get(spec_ref, {}): + _prot_acc = _input_by_ref[spec_ref].get("_protein_accessions", np.array([], dtype=object)) + # or lookup from template by spectrum_reference + result_rows.append({ ... - "protein_accessions": np.array([], dtype=object), + "protein_accessions": _prot_acc,This would require storing
protein_accessionsin_input_by_refduring the pre-filter step at lines 776-786.🤖 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 881 - 883, The protein_accessions field in the output DataFrame is unconditionally set to an empty array, discarding the original protein accession mappings from the input. To fix this, first modify the pre-filter step around lines 776-786 to store the original protein_accessions from the input DataFrame row into the _input_by_ref dictionary (similar to how psm_metavalues is preserved). Then, in the output DataFrame construction around lines 881-883, replace the empty array assignment for protein_accessions with a reference to the stored value from _input_by_ref, ensuring the original protein-to-peptide associations are preserved in the output.onsite/decoy_flr.py (2)
88-95:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDivision by zero when leading sites are all decoys.
When
cum_target(which equalsranks - cum_decoy) is zero for some prefix of the ranking—i.e., the top-ranked site(s) are all decoys—line 94 divides by zero, producinginf. The subsequentnp.minimum(flr_raw, 1.0)does not cap infinity (sinceinf > 1.0evaluates toTrue,minimumkeepsinf).This can occur in edge cases where the highest-confidence localizations are decoy sites (e.g., Ala winning over S/T/Y in ambiguous peptides).
🐛 Proposed fix: guard against zero denominator
ratio = (t_c / x_c) if x_c > 0 else 0.0 - flr_raw = (ratio * cum_decoy) / (ranks - cum_decoy) + denom = ranks - cum_decoy + with np.errstate(divide='ignore', invalid='ignore'): + flr_raw = np.where(denom > 0, (ratio * cum_decoy) / denom, 1.0) flr_raw = np.minimum(flr_raw, 1.0) # an FLR cannot exceed 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/decoy_flr.py` around lines 88 - 95, The flr_raw calculation divides by (ranks - cum_decoy) which can be zero when the top-ranked sites are all decoys, resulting in infinity values that cannot be properly capped by np.minimum. Add a guard condition to prevent division by zero by checking if (ranks - cum_decoy) equals zero before the division in the flr_raw assignment. When the denominator would be zero, set flr_raw to an appropriate value (such as 1.0, representing maximum false localization rate). This ensures the result stays within the valid range [0, 1] and handles the edge case where the highest-confidence localizations are decoy sites.
78-95:⚠️ Potential issue | 🔴 CriticalFix test parameter type and expected FLR values for new formula.
The test at line 183 passes
keep = {"s1", "s2", "s4", "s5"}(string set), but the implementation at line 326 expectskeep_refsto contain(spectrum_ref, unmod_seq)tuples. The test will fail with a type error; convertkeepto a tuple set.Additionally, the test assertion at line 30 expects
flr_raw = [0, 0, 0, 0, 0.4]based on the old formula comment at line 29 (2*(4/4)*1/5), but the new formula yields(4/4)*1/(5-1) = 0.25for rank 5. Update the assertion to match the new formula. The comment at line 191 ("factor 2*(4/1)=8") also references the old formula and needs updating.🤖 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/decoy_flr.py` around lines 78 - 95, The test at line 183 passes keep as a string set but the implementation expects keep_refs to contain (spectrum_ref, unmod_seq) tuples, causing a type error. Convert the keep parameter from a string set to a tuple set with the appropriate tuple structure. Additionally, update the test assertion at line 30 for flr_raw from [0, 0, 0, 0, 0.4] to match the new FLR formula (ratio * cum_decoy) / (ranks - cum_decoy), which yields 0.25 instead of 0.4 for rank 5. Also update the comment at line 191 that references the old formula multiplier to reflect the new formula calculation.
🤖 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/decoy_flr.py`:
- Around line 360-362: The comment above the collapsed.sort method call
references "bin score to 2 dp" but the actual sort key no longer includes the
round(t[0], 2) operation - it only sorts by the raw score t[0] and PSM count
t[2]. Update the comment to accurately reflect the current sorting behavior by
removing the reference to 2-decimal precision binning, or replace it with a
comment that describes what the code actually does: sorting by score and
tie-breaking by supporting-PSM count without any binning.
In `@tests/test_id_io.py`:
- Line 996: The assertion on line 996 uses equality comparison with boolean
literals (== False) which violates Ruff E712. Replace the equality check against
False by casting the value to bool() and using identity comparison (is) instead.
The same fix should be applied to the assertion on line 1046. This maintains
scalar compatibility while satisfying the Ruff linter rule.
---
Outside diff comments:
In `@onsite/decoy_flr.py`:
- Around line 88-95: The flr_raw calculation divides by (ranks - cum_decoy)
which can be zero when the top-ranked sites are all decoys, resulting in
infinity values that cannot be properly capped by np.minimum. Add a guard
condition to prevent division by zero by checking if (ranks - cum_decoy) equals
zero before the division in the flr_raw assignment. When the denominator would
be zero, set flr_raw to an appropriate value (such as 1.0, representing maximum
false localization rate). This ensures the result stays within the valid range
[0, 1] and handles the edge case where the highest-confidence localizations are
decoy sites.
- Around line 78-95: The test at line 183 passes keep as a string set but the
implementation expects keep_refs to contain (spectrum_ref, unmod_seq) tuples,
causing a type error. Convert the keep parameter from a string set to a tuple
set with the appropriate tuple structure. Additionally, update the test
assertion at line 30 for flr_raw from [0, 0, 0, 0, 0.4] to match the new FLR
formula (ratio * cum_decoy) / (ranks - cum_decoy), which yields 0.25 instead of
0.4 for rank 5. Also update the comment at line 191 that references the old
formula multiplier to reflect the new formula calculation.
In `@onsite/id_io.py`:
- Around line 292-296: The aa_before and aa_after fields on lines 293-294 do not
defensively handle bytes returned by getAABefore() and getAAAfter() methods.
Apply the same bytes-handling pattern already demonstrated at line 306 by
checking if the return value is bytes using isinstance() and calling decode() if
needed, otherwise use the value as-is. Replace the str() calls around
getAABefore() and getAAAfter() with conditional expressions that properly decode
bytes to strings.
In `@onsite/lucxor/cli.py`:
- Around line 881-883: The protein_accessions field in the output DataFrame is
unconditionally set to an empty array, discarding the original protein accession
mappings from the input. To fix this, first modify the pre-filter step around
lines 776-786 to store the original protein_accessions from the input DataFrame
row into the _input_by_ref dictionary (similar to how psm_metavalues is
preserved). Then, in the output DataFrame construction around lines 881-883,
replace the empty array assignment for protein_accessions with a reference to
the stored value from _input_by_ref, ensuring the original protein-to-peptide
associations are preserved in the output.
🪄 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: fcef327a-a337-40d2-9324-29176b216d19
📒 Files selected for processing (8)
onsite/ascore/ascore.pyonsite/ascore/cli.pyonsite/decoy_flr.pyonsite/id_io.pyonsite/idparquet.pyonsite/lucxor/cli.pyonsite/phosphors/phosphors.pytests/test_id_io.py
🚧 Files skipped from review as they are similar to previous changes (3)
- onsite/ascore/cli.py
- onsite/ascore/ascore.py
- onsite/idparquet.py
|
|
||
| psms2, *_ = load_dataframes(out) | ||
| assert len(psms2) == 1 | ||
| assert psms2["is_decoy"].iloc[0] is False or psms2["is_decoy"].iloc[0] == False, ( |
Contributor
There was a problem hiding this comment.
Replace == True/False assertions to satisfy Ruff E712 and keep scalar compatibility.
Line 996 and Line 1046 use equality checks against boolean literals, which Ruff flags (E712). Cast to bool(...) and assert identity instead.
Suggested patch
- assert psms2["is_decoy"].iloc[0] is False or psms2["is_decoy"].iloc[0] == False, (
+ assert bool(psms2["is_decoy"].iloc[0]) is False, (
f"Expected is_decoy=False for NaN input, got {psms2['is_decoy'].iloc[0]!r}"
)
...
- assert row2["is_decoy"] is True or row2["is_decoy"] == True, (
+ assert bool(row2["is_decoy"]) is True, (
f"Expected is_decoy=True after round-trip, got {row2['is_decoy']!r}"
)Also applies to: 1046-1046
🧰 Tools
🪛 Ruff (0.15.17)
[error] 996-996: Avoid equality comparisons to False; use not psms2["is_decoy"].iloc[0]: for false checks
Replace with not psms2["is_decoy"].iloc[0]
(E712)
🤖 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 `@tests/test_id_io.py` at line 996, The assertion on line 996 uses equality
comparison with boolean literals (== False) which violates Ruff E712. Replace
the equality check against False by casting the value to bool() and using
identity comparison (is) instead. The same fix should be applied to the
assertion on line 1046. This maintains scalar compatibility while satisfying the
Ruff linter rule.
Source: Linters/SAST tools
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_decoy_flr.py`:
- Around line 29-32: The comment on the line before the assert_allclose call for
curve["flr_raw"] contains an arithmetic error. The comment states the
calculation equals 0.4, but the correct result of (4/4)*1/(5-1) is 0.25, which
matches the actual assertion value in
np.testing.assert_allclose(curve["flr_raw"], [0, 0, 0, 0, 0.25]). Update the
comment to show the correct result of 0.25 instead of 0.4 so the documentation
matches the test expectation.
🪄 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: dfeaad05-3f4b-4f31-9710-bd6dab57b07d
📒 Files selected for processing (4)
onsite/decoy_flr.pyonsite/id_io.pyonsite/idparquet.pytests/test_decoy_flr.py
🚧 Files skipped from review as they are similar to previous changes (2)
- onsite/idparquet.py
- onsite/id_io.py
Comment on lines
+29
to
+32
| # flr_raw: 0,0,0,0, (4/4)*1/(5-1) = 0.4 | ||
| np.testing.assert_allclose(curve["flr_raw"], [0, 0, 0, 0, 0.25]) | ||
| # qval == flr_raw here (already monotone non-decreasing) | ||
| np.testing.assert_allclose(curve["qval"], [0, 0, 0, 0, 0.4]) | ||
| np.testing.assert_allclose(curve["qval"], [0, 0, 0, 0, 0.25]) |
Contributor
There was a problem hiding this comment.
Typo in comment: arithmetic result is wrong.
The comment says (4/4)*1/(5-1) = 0.4 but the correct result is 1*1/4 = 0.25, which matches the assertion. The = 0.4 should be = 0.25.
📝 Suggested fix
- # flr_raw: 0,0,0,0, (4/4)*1/(5-1) = 0.4
+ # flr_raw: 0,0,0,0, (4/4)*1/(5-1) = 0.25📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # flr_raw: 0,0,0,0, (4/4)*1/(5-1) = 0.4 | |
| np.testing.assert_allclose(curve["flr_raw"], [0, 0, 0, 0, 0.25]) | |
| # qval == flr_raw here (already monotone non-decreasing) | |
| np.testing.assert_allclose(curve["qval"], [0, 0, 0, 0, 0.4]) | |
| np.testing.assert_allclose(curve["qval"], [0, 0, 0, 0, 0.25]) | |
| # flr_raw: 0,0,0,0, (4/4)*1/(5-1) = 0.25 | |
| np.testing.assert_allclose(curve["flr_raw"], [0, 0, 0, 0, 0.25]) | |
| # qval == flr_raw here (already monotone non-decreasing) | |
| np.testing.assert_allclose(curve["qval"], [0, 0, 0, 0, 0.25]) |
🤖 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 `@tests/test_decoy_flr.py` around lines 29 - 32, The comment on the line before
the assert_allclose call for curve["flr_raw"] contains an arithmetic error. The
comment states the calculation equals 0.4, but the correct result of
(4/4)*1/(5-1) is 0.25, which matches the actual assertion value in
np.testing.assert_allclose(curve["flr_raw"], [0, 0, 0, 0, 0.25]). Update the
comment to show the correct result of 0.25 instead of 0.4 so the documentation
matches the test expectation.
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
2 participants
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
Generalizes the identification I/O so the AScore/PhosphoRS/LucXor pipeline reads and writes idXML, mzIdentML, or idParquet by file extension, normalized to the existing idParquet pandas-DataFrame model. Also re-applies the localizer hot-path performance optimizations on top of the current idParquet architecture.
What's new
onsite/id_io.py(new):detect_format,load_identifications,save_identifications.load_identifications: idParquet →load_dataframes(native); idXML/mzid → pyOpenMS load →psms_df(peptidoform via UNIMOD notation,protein_accessions,modifications, and typedpsm_metavaluescarrying all score keys).save_identifications: dispatch by extension. mzid store strips the non-UNIMOD customPhosphoDecoymodification fromSearchParameters.variable_modifications(otherwiseMzIdentMLFile.storerejectsUNIMOD:99913); the decoy modification stays on the hits and round-trips back intact.onsite/idparquet.py: newsave_psms_from_scratchso idXML/mzid → idParquet works without an existing idParquet template (main'ssave_dataframesonly copy-updates a source).onsitecmerge route user-facing input/output through the format layer; the native idParquet→idParquet path is unchanged (copy-template), and internal per-tool intermediates stay idParquet.ascore/cli.pyshared-instance tweak was intentionally dropped — main refactored that CLI to DataFrame groups, so it no longer applies; the core AScore win lives inascore.pyand is preserved.Testing
Full suite 172 passed, 0 failed. New
tests/test_id_io.pycovers: format detection, idParquet/idXML/mzid load→df, save round-trips for all three formats, the from-scratch idParquet writer, score-key (AScore_site_scores) survival across every output format, and a realPhosphoDecoy(A)mzid round-trip.Notes / follow-ups (non-blocking)
save_psms_from_scratchcurrently leaves entirely-empty columns null-typed rather than fixture-typed (e.g.mz_array,cv_params). It is consumable and idempotent for this pipeline; emitting an explicit pyarrow schema would make it type-faithful for strict external idParquet consumers. Tracked as a follow-up.search_params.parquetfrom-scratch stub writes a subset of columns (the native passthrough copies the full file);template_dfon the from-scratch branch is reserved for that follow-up.Supersedes #51 (the pyOpenMS-object mzIdentML approach), which predated main's idParquet migration.
Summary by CodeRabbit
save_psms_from_scratchfor creating new idParquet outputs.