Skip to content

patch for KeyError when attempting a direct index lookup for Thermo-s… - #14

Closed
animesh wants to merge 1 commit into
SimpleNumber:masterfrom
animesh:master
Closed

patch for KeyError when attempting a direct index lookup for Thermo-s…#14
animesh wants to merge 1 commit into
SimpleNumber:masterfrom
animesh:master

Conversation

@animesh

@animesh animesh commented Sep 2, 2026

Copy link
Copy Markdown

…tyle composite spectrum identifiers in mzML files where scan identifiers are formatted strictly as scan=X or integer scan numbers

…tyle composite spectrum identifiers in mzML files where scan identifiers are formatted strictly as scan=X or integer scan numbers
Copilot AI lite review requested due to automatic review settings September 2, 2026 15:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new fallback still risks KeyError because it only tries the numeric scan as a string, which may not match readers that expect an integer scan number.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens spectrum lookup in preprocess_spectrum to avoid KeyError when mzML spectrum identifiers are provided in alternate “scan=X” or numeric scan-number formats.

Changes:

  • Wraps reader[spec_id] access in a try/except KeyError and attempts to extract a scan number from spec_id.
  • Adds fallback lookups using "scan=<n>" and <n> when the direct lookup fails.
File summaries
File Description
AA_stat/localization.py Adds fallback spectrum ID parsing/lookup to handle scan-based IDs and numeric scan numbers.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread AA_stat/localization.py
@levitsky

levitsky commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Hi,

Thanks for this contribution. I'm having trouble understanding the exact use case. Do you have an example of input for AA_stat that requires this fix? I would appreciate if you could show a snippet of PSM and mzML files, and what software produced them.

@animesh

animesh commented Sep 4, 2026

Copy link
Copy Markdown
Author

Hi @levitsky , thanks for AA_stat, use it all the time! Huge fan!! This is just a workaround i had to employ to make my opensearch nextflow pipeline https://github.com/animesh/opensearch work on orbitrap data, otherwise i get this error
log.txt for example?

@levitsky

levitsky commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the kind words @animesh and I'm happy it is useful! Your pipeline looks great!
I've been trying to run it locally and I have not seen the need for the patch, though. AA_stat seems to work fine on my files (I also tried the example files linked in your repo, but for me AA_stat doesn't do any localization on them). If you don't mind, I would be happy to see an example where the patch is needed.

@animesh

animesh commented Sep 9, 2026

Copy link
Copy Markdown
Author

Thanks for your kind words @levitsky 🙏 Most probably it is a local issue then, can you share the commands you are using to run AA_stat over the Fragpipe's output, maybe my pipeline is missing some argument/switches? I must note that the patch is required only for the orbitrap data, so i am assuming you tried the test.raw.tar files?

@levitsky

levitsky commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

I actually didn't run AA_stat manually but directly ran your Nextflow pipeline according to README instructions. I did it first on the test.BSA.raw.tar files, but there my search didn't find any mass shifts after filtering, so localization was not performed (technically AA_stat run passed correctly but there was no chance for it to break in localization). I then took bigger files from Orbitrap, and your pipeline completed fine on them too. I verified that AA_stat log looks normal, including localization. I followed your installation instructions but skipped the patching of localization.py.

I think the error could be triggered by specific raw files or specific version of FragPipe or its components. But in my tests the spectrum IDs in pepXML files look like spectrum="SDS_01_01.1842.1842.2" and in the calibrated mzML files they look like id="controllerType=0 controllerNumber=1 scan=1842". AA_stat already knows how to translate the former into the latter, so everything works fine for me. I used latest released versions of FragPipe and MSFragger.

@animesh

animesh commented Sep 9, 2026

Copy link
Copy Markdown
Author

Thanks for trying the pipelie @levitsky , glad that it worked fine at your end! I still need the patch though, probably something to do with my setup. Sorry for wasting your time though! I will close this and look forward to more cool updates from AA_stat 👍🏽

@animesh animesh closed this Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants