make reindexing the default #5581
Conversation
|
Can this please go into SearchEngineBase? |
|
rebuild jenkins |
src/utils/MSFraggerAdapter.cpp
Outdated
| Param param_pi = PeptideIndexing().getParameters(); | ||
| // overwrite with search engine specific defaults | ||
| //param_pi.setValue(const std::string &key, const ParamValue &value) TODO: set search engine defaults | ||
| registerPeptideIndexingParameter_(param_pi); |
There was a problem hiding this comment.
this looks like its a lot of boilerplate for every search adapter.
Could also go into SearchEngineBase, using some override methods.
There was a problem hiding this comment.
really? I would have thought it is fine as it is one line of code if you don't need to do any adaptions.
registerPeptideIndexingParameter_(PeptideIndexing().getParameters());
src/utils/MSFraggerAdapter.cpp
Outdated
| // reindex ids | ||
| if (getStringOption_("reindex") == "true") | ||
| { | ||
| if (auto ret = reindex_(protein_identifications, peptide_identifications); ret != EXECUTION_OK) return ret; |
There was a problem hiding this comment.
Also too much boilerplate for my taste.
reindex_ could check the flag's reindex state and handle the outer if, right?
The inner if is a bit more tricky due to the conditional return here.
I can only think of throwing an exception, say throw TOPPReturnException(ERROR_CODE), if indexing did not work (as a means to "return") and catching that exception here: https://github.com/OpenMS/OpenMS/blob/develop/src/openms/source/APPLICATIONS/TOPPBase.cpp#L451 and set result according to the value stored in the exception. That would actually make some other places in TOPP tools cleaner as well.
All this can go inside reindex_.
Maybe there is an even better way though...
There was a problem hiding this comment.
Originally I also thought about moving more to SearchEngineBase e.g., making parameter mandatory for all search engines etc. but in the end you might also want to use e.g. SimpleSearchEngine base which does it automatically without this parameter. Other third-party search engines might give you peptides that can't be indexed for whatever reason.
So in the end you anyway need a reviewer that will point out that you need to add some extra lines if you implement a new adapter. Being it 5 or 2 + try/catch code doesn't make a difference to me (and probably doesn't in terms of lines of code).
There was a problem hiding this comment.
But I agree that with some extra work (adding new virtual members as customization points) it could be reduced. In light of time and resources, maybe later ;)
| <UserParam type="string" name="CometAdapter:1:log" value=""/> | ||
| <UserParam type="int" name="CometAdapter:1:debug" value="0"/> | ||
| <UserParam type="int" name="CometAdapter:1:threads" value="1"/> | ||
| <UserParam type="string" name="CometAdapter:1:no_progress" value="false"/> | ||
| <UserParam type="string" name="CometAdapter:1:force" value="true"/> | ||
| <UserParam type="string" name="CometAdapter:1:test" value="true"/> | ||
| <UserParam type="string" name="CometAdapter:1:decoy_string" value=""/> |
There was a problem hiding this comment.
probalby related to the way we store these user-values, but using only the name of the leaf, and not the full path seems dangerous.
Any insights @Waschi97 ?
There was a problem hiding this comment.
Right, an issue regarding reproducibility.
|
thanks for the comments. I will try to reduce the boilerplate code a bit more. |
Description
For all workflows we call PeptideIndexer after a database search. This PR makes that default behavior by incorporating it into the search engine adapter.
pro:
Checklist:
How can I get additional information on failed tests during CI:
If your PR is failing you can check out
Note: