Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for pepxml files with search_engine = Percolator #3278

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

poshul
Copy link

@poshul poshul commented Dec 13, 2024

OpenMS' Percolator adapter saves the search_engine as Percolator directly. As a result it's not possible to load OpenMS generated pepxmls that have Percolator postprocessing into Skyline. This patch fixes that.

Copy link
Member

@chambm chambm left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. It needs a test added though. Easiest way would be taking one of the existing Percolated pepXMLs and changing the search_engine (and removing post-processor parameter if OpenMS doesn't set that). Better would be getting a real OpenMS pepXML but cutting it down to be <1MB. Is it clear enough how to add a test when looking at the tests/Jamfile.jam? (when you run the test the first time it will create a reference/xxx.observed file; just change that .check after reviewing it to be sure it's correct and then the test should pass).

@@ -419,7 +426,8 @@ void PepXMLreader::startElement(const XML_Char* name, const XML_Char** attr)
(analysisType_ == PROTEOME_DISCOVERER_ANALYSIS && scoreType_ == MASCOT_IONS_SCORE && score_name == "exp-value") ||
(analysisType_ == MORPHEUS_ANALYSIS && score_name == "psm q-value") ||
(analysisType_ == MSGF_ANALYSIS && score_name == "qvalue") ||
(analysisType_ == CRUX_ANALYSIS && score_name == "percolator_qvalue")) {
(analysisType_ == CRUX_ANALYSIS && score_name == "percolator_qvalue")
(analysisType_ == PERCOLATOR_ANALYSIS)) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a missing || operator? How did this work?

Copy link
Author

Choose a reason for hiding this comment

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

And this is why I shouldnt push changes on a friday afternoon. Fixed.

@poshul
Copy link
Author

poshul commented Dec 16, 2024

Thanks for the PR. It needs a test added though. Easiest way would be taking one of the existing Percolated pepXMLs and changing the search_engine (and removing post-processor parameter if OpenMS doesn't set that). Better would be getting a real OpenMS pepXML but cutting it down to be <1MB. Is it clear enough how to add a test when looking at the tests/Jamfile.jam? (when you run the test the first time it will create a reference/xxx.observed file; just change that .check after reviewing it to be sure it's correct and then the test should pass).

I've been working on the test for this, a meaningful search with decoys, in a small mzml file, that produces better results than "nothing found" is proving to be a pain. I'm not going to have a chance to circle back to this before the holidays, so I'm putting a note in my calendar to come back in early Jan.

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.

2 participants