-
Notifications
You must be signed in to change notification settings - Fork 5
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
SearchDatabase Reupload für Codereview #140
base: develop
Are you sure you want to change the base?
Conversation
* @param experiment Input MSExperiment containing of MS2 Spectra | ||
* @param candidates Output vector of found candidates with the index of MSSpectrum in experiment | ||
*/ | ||
void search(MSExperiment& experiment, std::vector<std::pair<std::vector<Candidate>, size_t>>& candidates) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very usefull to make the functions const, makes it easier to parall. for other people :)
std::vector<SearchDatabase::Peptide_> generate_peptides_(const std::vector<FASTAFile::FASTAEntry>& entries) const; | ||
/// Merges presorted Chunks of Peptide-Fragments inplace | ||
void fragment_merge_(int first, int last, const std::vector<int>& chunks, std::vector<SearchDatabase::Fragment_>& input) const; | ||
///generates sortet vector with all theoretical Fragments for all theoretical Peptides |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very small addition: sortet -> sorted
return all_peptides; | ||
} | ||
|
||
void SearchDatabase::fragment_merge_(int first, int last, const std::vector<int>& chunks, std::vector<SearchDatabase::Fragment_>& input) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const _ & for first and last possible?
if (last - first > 1) | ||
{ | ||
int mid = first + (last-first) / 2; | ||
#pragma omp parallel sections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea for optimisation
std::vector<Fragment_> all_frags; | ||
std::vector<int> chunk_start = {0}; //for fragment_merge need start and end of presorted chunks | ||
|
||
for (size_t i = 0; i < all_peptides_.size(); i++) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Para possible? emplace_back() could be changed to [] operator for no conflict
#include <OpenMS/KERNEL/Peak1D.h> | ||
#include <OpenMS/CHEMISTRY/ProteaseDigestion.h> | ||
#include <OpenMS/CHEMISTRY/TheoreticalSpectrumGenerator.h> | ||
#include <vector> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coding convention (alph. order)
std::vector<SearchDatabase::Peptide_> generate_peptides_(const std::vector<FASTAFile::FASTAEntry>& entries) const; | ||
/// Merges presorted Chunks of Peptide-Fragments inplace | ||
void fragment_merge_(int first, int last, const std::vector<int>& chunks, std::vector<SearchDatabase::Fragment_>& input) const; | ||
///generates sortet vector with all theoretical Fragments for all theoretical Peptides |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
///generates sortet vector with all theoretical Fragments for all theoretical Peptides | |
///generates sorted vector with all theoretical Fragments for all theoretical Peptides |
#include <OpenMS/CHEMISTRY/AASequence.h> | ||
#include <OpenMS/DATASTRUCTURES/DefaultParamHandler.h> | ||
#include <OpenMS/FORMAT/FASTAFile.h> | ||
#include <OpenMS/KERNEL/MSExperiment.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include <OpenMS/KERNEL/MSExperiment.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesnt work with forwarding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you include the header in your cpp?
(because MSExperiment must be known when compiling SearchDatabase.cpp)
namespace OpenMS | ||
{ | ||
/** @brief Creating a two level tree from a given FASTAFile by generating the theoretical peptides and their b-y-ions and sorting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namespace OpenMS | |
{ | |
/** @brief Creating a two level tree from a given FASTAFile by generating the theoretical peptides and their b-y-ions and sorting | |
namespace OpenMS | |
{ | |
class MSExperiment; | |
/** @brief Creating a two level tree from a given FASTAFile by generating the theoretical peptides and their b-y-ions and sorting |
namespace OpenMS | ||
{ | ||
/** @brief Creating a two level tree from a given FASTAFile by generating the theoretical peptides and their b-y-ions and sorting | ||
* them in the outer tree by fragment-MZ and the inner tree by precursor-MZ. Search function to find candidates to experimental MS2 sepctra |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add link to blog of sagems
|
||
* @param spectrum Input MS2 Spectrum | ||
* @param candidates Output vector of found candidatest | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens for an MS1 spectrum
AASequence sequence_; | ||
size_t protein_index_; | ||
double peptide_mz_; | ||
Peptide_() = default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check if needed;\iff needed then intialize the members
size_t protein_index_; | ||
double peptide_mz_; | ||
Peptide_() = default; | ||
Peptide_(const Peptide_&) = default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I claim: move ctor is deleted but should exist
std::vector<SearchDatabase::Fragment_> generate_fragments_() const; | ||
|
||
std::string digestor_enzyme_; | ||
size_t missed_cleavages_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size_t missed_cleavages_; | |
size_t missed_cleavages_; ///< number of missed cleavages |
size_t count_found = 0; | ||
size_t count_rounds = 0; | ||
|
||
auto prec_unit_cond = precursor_mz_tolerance_unit_ == "Da"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto prec_unit_cond = precursor_mz_tolerance_unit_ == "Da"; | |
auto prec_unit_cond = precursor_mz_tolerance_unit_ == UNIT_DA; |
|
||
return all_frags; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
char* UNIT_DA = "Da"; |
ProteaseDB::getInstance()->getAllNames(all_enzymes); | ||
vector<String> all_mods; | ||
ModificationsDB::getInstance()->getAllSearchModifications(all_mods); | ||
vector<string> tolerance_units{"Da", "ppm"}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vector<string> tolerance_units{"Da", "ppm"}; | |
vector<string> tolerance_units{UNIT_DA, "ppm"}; |
defaults_.setValue("peptide_max_length", 50, "maximal peptide length for database"); | ||
defaults_.setValue("fragment_min_mz", 150, "minimal fragment mz for database"); | ||
defaults_.setValue("fragment_max_mz", 2000, "maximal fragment mz for database"); | ||
defaults_.setValue("precursor_mz_tolerance", 2, "tolerance for precursor-MZ in search"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaults_.setValue("precursor_mz_tolerance", 2, "tolerance for precursor-MZ in search"); | |
defaults_.setValue("precursor_mz_tolerance", 2, "tolerance for precursor-m/z in search"); |
#pragma omp parallel | ||
{ | ||
// sorting the fragments of each bucket by the precursor-MZ | ||
#pragma omp for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#pragma omp parallel | |
{ | |
// sorting the fragments of each bucket by the precursor-MZ | |
#pragma omp for | |
// sorting the fragments of each bucket by the precursor-MZ | |
#pragma omp parallel for | |
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this { in line 200 needed. Cause our code only works without it?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, please re-add it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or delete the corresponding }
after the for loop (probably even better this way)
…\n added test to check if Datasructure is sorted \n added UNIT_DA and UNIT_PPM to Constants.h
Co-authored-by: Timo Sachsenberg <timo.sachsenberg@uni-tuebingen.de>
Co-authored-by: Timo Sachsenberg <timo.sachsenberg@uni-tuebingen.de>
Co-authored-by: Timo Sachsenberg <timo.sachsenberg@uni-tuebingen.de>
Co-authored-by: Timo Sachsenberg <timo.sachsenberg@uni-tuebingen.de>
Co-authored-by: Timo Sachsenberg <timo.sachsenberg@uni-tuebingen.de>
Co-authored-by: Julianus Pfeuffer <pfeuffer@informatik.uni-tuebingen.de>
Co-authored-by: Julianus Pfeuffer <pfeuffer@informatik.uni-tuebingen.de>
Co-authored-by: Julianus Pfeuffer <pfeuffer@informatik.uni-tuebingen.de>
Co-authored-by: Julianus Pfeuffer <pfeuffer@informatik.uni-tuebingen.de>
Co-authored-by: Julianus Pfeuffer <pfeuffer@informatik.uni-tuebingen.de>
…ead of AASequence, changed name to FragmentIndex
Description
Checklist
How can I get additional information on failed tests during CI
Click to expand
If your PR is failing you can check outIf you click in the column that lists the failed tests you will get detailed error messages.
Advanced commands (admins / reviewer only)
Click to expand
/reformat
(experimental) applies the clang-format style changes as additional commit. Note: your branch must have a different name (e.g., yourrepo:feature/XYZ) than the receiving branch (e.g., OpenMS:develop). Otherwise, reformat fails to push.rebuild jenkins
will retrigger Jenkins-based CI builds