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

Remove EXTRACTPSMFEATURES/IDScoreswitcher from workflow #448

Open
ypriverol opened this issue Nov 28, 2024 · 12 comments
Open

Remove EXTRACTPSMFEATURES/IDScoreswitcher from workflow #448

ypriverol opened this issue Nov 28, 2024 · 12 comments
Assignees
Labels
enhancement New feature or request

Comments

@ypriverol
Copy link
Member

Description of feature

In the latest OpenMS (dev) version, the FeatureExtraction process happens within each Search engine adapter MSGFAdapter, SAGEAdapter, etc. We don't have to rerun this step for every Search engine, then this step can be removed it.

Thoughts @timosachsenberg @jpfeuffer before starting.

@ypriverol ypriverol added the enhancement New feature or request label Nov 28, 2024
@ypriverol ypriverol self-assigned this Nov 28, 2024
@jpfeuffer
Copy link
Collaborator

Also remove IDScoreswitcher. I think @timosachsenberg merged my simplification changes

@timosachsenberg
Copy link

I defnitely merged also some IDScoreSwitcher changes but I did not test it in the context of quantMS. I think it is a valuable change but should be done with two PRs that are each tested on some small data.

@ypriverol ypriverol changed the title Remove EXTRACTPSMFEATURES from workflow Remove EXTRACTPSMFEATURES/IDScoreswitcher from workflow Nov 28, 2024
@ypriverol
Copy link
Member Author

What about changing all the tests in singularity and docker to point to OpenMS Dev. Which we sync the test and new deployments and changes in OpenMS with quantms more clearly-tidy. ?

@ypriverol
Copy link
Member Author

Related to this topic, we have one flag called posterior_probabilities = 'percolator' this flag defines the posterior_probabilites used in the workflow. However, I have never run this flag without that value. Can we remove it @daichengxin @jpfeuffer? We should always use Percolator probabilities.

@daichengxin
Copy link
Collaborator

We could remove this parameter and the corresponding modules if we make sure always use the Percolator probability. But I don't see any harm in leaving it there. I don't know if any other users use it, at least we

@ypriverol
Copy link
Member Author

No harm, but confusing if is always used with same value.

@jpfeuffer
Copy link
Collaborator

Fine with me. Percolator will not work for very small data though.

@ypriverol
Copy link
Member Author

How small are we talking here?

@jpfeuffer
Copy link
Collaborator

Hard to say. @timosachsenberg knows better.
1000 PSMs after FDR filter?

@ypriverol
Copy link
Member Author

Ok, quantms is not designed for that anyway. I haven't seen any msruns with less that 1k psms.

@timosachsenberg
Copy link

Hard to say. @timosachsenberg knows better. 1000 PSMs after FDR filter?

Yeah maybe 400 would still work.
I think we added it as fallback to be able to analyze small studies or blanks (!) if we do per file percolator

@ypriverol
Copy link
Member Author

ypriverol commented Nov 28, 2024

If people don't use it really, it should be here unless someone needs it, BTW, the pipeline becomes more complex for something that hardly happens.

BTW, @timosachsenberg @jpfeuffer @daichengxin The PR removing posterior_probabilities = 'percolator' is ready here #450

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants