SearchDatabase Reupload für Codereview #140

Maxalcer · 2023-05-04T13:08:43Z

Description

Checklist

Make sure that you are listed in the AUTHORS file
Add relevant changes and new features to the CHANGELOG file
I have commented my code, particularly in hard-to-understand areas
New and existing unit tests pass locally with my changes
Updated or added python bindings for changed or new classes (Tick if no updates were necessary.)

How can I get additional information on failed tests during CI

Click to expand

If your PR is failing you can check out

The details of the action statuses at the end of the PR or the "Checks" tab.
http://cdash.openms.de/index.php?project=OpenMS and look for your PR. Use the "Show filters" capability on the top right to search for your PR number.
If 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.
setting the label "NoJenkins" will skip tests for this PR on jenkins (saves resources e.g., on edits that do not affect tests)
commenting with rebuild jenkins will retrigger Jenkins-based CI builds

⚠️ Note: Once you opened a PR try to minimize the number of pushes to it as every push will trigger CI (automated builds and test) and is rather heavy on our infrastructure (e.g., if several pushes per day are performed).

ksenikrva · 2023-05-04T13:23:29Z

src/openms/include/OpenMS/ANALYSIS/ID/SearchDatabase.h

+     * @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;


Very usefull to make the functions const, makes it easier to parall. for other people :)

ksenikrva · 2023-05-04T13:27:25Z

src/openms/include/OpenMS/ANALYSIS/ID/SearchDatabase.h

+  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


very small addition: sortet -> sorted

ksenikrva · 2023-05-04T13:45:02Z

src/openms/source/ANALYSIS/ID/SearchDatabase.cpp

+  return all_peptides;
+}
+
+void SearchDatabase::fragment_merge_(int first, int last, const std::vector<int>& chunks, std::vector<SearchDatabase::Fragment_>& input) const


const _ & for first and last possible?

ksenikrva · 2023-05-04T13:51:17Z

src/openms/source/ANALYSIS/ID/SearchDatabase.cpp

+  if (last - first > 1)
+  {
+    int mid = first + (last-first) / 2;
+    #pragma omp parallel sections


Nice idea for optimisation

ksenikrva · 2023-05-04T14:00:05Z

src/openms/source/ANALYSIS/ID/SearchDatabase.cpp

+  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++)


Para possible? emplace_back() could be changed to [] operator for no conflict

ksenikrva · 2023-05-04T14:10:05Z

src/tests/class_tests/openms/source/SearchDatabase_test.cpp

+#include <OpenMS/KERNEL/Peak1D.h>
+#include <OpenMS/CHEMISTRY/ProteaseDigestion.h>
+#include <OpenMS/CHEMISTRY/TheoreticalSpectrumGenerator.h>
+#include <vector>


Coding convention (alph. order)

cbielow · 2023-05-05T07:31:18Z

src/openms/include/OpenMS/ANALYSIS/ID/SearchDatabase.h

+  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


Suggested change

///generates sortet vector with all theoretical Fragments for all theoretical Peptides

///generates sorted vector with all theoretical Fragments for all theoretical Peptides

cbielow · 2023-05-05T07:33:23Z

src/openms/include/OpenMS/ANALYSIS/ID/SearchDatabase.h

+#include <OpenMS/CHEMISTRY/AASequence.h>
+#include <OpenMS/DATASTRUCTURES/DefaultParamHandler.h>
+#include <OpenMS/FORMAT/FASTAFile.h>
+#include <OpenMS/KERNEL/MSExperiment.h>


Suggested change

#include <OpenMS/KERNEL/MSExperiment.h>

Doesnt work with forwarding

did you include the header in your cpp?
(because MSExperiment must be known when compiling SearchDatabase.cpp)

cbielow · 2023-05-05T07:33:41Z

src/openms/include/OpenMS/ANALYSIS/ID/SearchDatabase.h

+namespace OpenMS
+{
+  /** @brief Creating a two level tree from a given FASTAFile by generating the theoretical peptides and their b-y-ions and sorting


Suggested change

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

cbielow · 2023-05-05T07:35:09Z

src/openms/include/OpenMS/ANALYSIS/ID/SearchDatabase.h

+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


add link to blog of sagems

cbielow · 2023-05-05T07:40:28Z

src/openms/include/OpenMS/ANALYSIS/ID/SearchDatabase.h

+
+     * @param spectrum Input MS2 Spectrum
+     * @param candidates Output vector of found candidatest
+    */


what happens for an MS1 spectrum

cbielow · 2023-05-05T07:42:51Z

src/openms/include/OpenMS/ANALYSIS/ID/SearchDatabase.h

+    AASequence sequence_;
+    size_t protein_index_;
+    double peptide_mz_;
+    Peptide_() = default;


check if needed;\iff needed then intialize the members

cbielow · 2023-05-05T07:45:05Z

src/openms/include/OpenMS/ANALYSIS/ID/SearchDatabase.h

+    size_t protein_index_;
+    double peptide_mz_;
+    Peptide_() = default;
+    Peptide_(const Peptide_&) = default;


I claim: move ctor is deleted but should exist

cbielow · 2023-05-05T07:46:41Z

src/openms/include/OpenMS/ANALYSIS/ID/SearchDatabase.h

+  std::vector<SearchDatabase::Fragment_> generate_fragments_() const;
+
+  std::string digestor_enzyme_;
+  size_t missed_cleavages_;


Suggested change

size_t missed_cleavages_;

size_t missed_cleavages_; ///< number of missed cleavages

cbielow · 2023-05-05T07:53:23Z

src/openms/source/ANALYSIS/ID/SearchDatabase.cpp

+  size_t count_found = 0;
+  size_t count_rounds = 0;
+
+  auto prec_unit_cond = precursor_mz_tolerance_unit_ == "Da";


Suggested change

auto prec_unit_cond = precursor_mz_tolerance_unit_ == "Da";

auto prec_unit_cond = precursor_mz_tolerance_unit_ == UNIT_DA;

cbielow · 2023-05-05T07:54:06Z

src/openms/source/ANALYSIS/ID/SearchDatabase.cpp

+
+  return all_frags;
+}
+


Suggested change

char* UNIT_DA = "Da";

cbielow · 2023-05-05T07:54:19Z

src/openms/source/ANALYSIS/ID/SearchDatabase.cpp

+  ProteaseDB::getInstance()->getAllNames(all_enzymes);
+  vector<String> all_mods;
+  ModificationsDB::getInstance()->getAllSearchModifications(all_mods);
+  vector<string> tolerance_units{"Da", "ppm"};


Suggested change

vector<string> tolerance_units{"Da", "ppm"};

vector<string> tolerance_units{UNIT_DA, "ppm"};

cbielow · 2023-05-05T07:56:22Z

src/openms/source/ANALYSIS/ID/SearchDatabase.cpp

+  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");


Suggested change

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");

cbielow · 2023-05-05T07:57:53Z

src/openms/source/ANALYSIS/ID/SearchDatabase.cpp

+  #pragma omp parallel
+  {
+    // sorting the fragments of each bucket by the precursor-MZ
+    #pragma omp for


Suggested change

#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

{

Is this { in line 200 needed. Cause our code only works without it?!

yes, please re-add it

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: Julianus Pfeuffer <pfeuffer@informatik.uni-tuebingen.de>

…ead of AASequence, changed name to FragmentIndex

Maxalcer added 2 commits May 4, 2023 15:07

Reupload für Codereview

da1b688

Modifikationen hinzugefügt

13ea132

ksenikrva reviewed May 4, 2023

View reviewed changes

cbielow reviewed May 5, 2023

View reviewed changes

src/openms/source/ANALYSIS/ID/SearchDatabase.cpp

return all_frags;

}

Copy link

Owner

cbielow May 5, 2023

Choose a reason for hiding this comment

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

Suggested change

char* UNIT_DA = "Da";

cbielow reviewed May 5, 2023

View reviewed changes

Maxalcer and others added 14 commits May 5, 2023 14:36

made changes based on code rewiev \n added test for tolerance in ppm …

31cee37

…\n added test to check if Datasructure is sorted \n added UNIT_DA and UNIT_PPM to Constants.h

Made Destructor not virtual

6c1077a

small doc change

2b73ddb

Co-authored-by: Timo Sachsenberg <timo.sachsenberg@uni-tuebingen.de>

removed destr. and constr. deklaration (not needed)

8526574

Co-authored-by: Timo Sachsenberg <timo.sachsenberg@uni-tuebingen.de>

removed frag constr.

c30c78f

Co-authored-by: Timo Sachsenberg <timo.sachsenberg@uni-tuebingen.de>

.empty() instead of == 0

d6e17f3

Co-authored-by: Timo Sachsenberg <timo.sachsenberg@uni-tuebingen.de>

.empty() instead of == 0

d8574dc

Co-authored-by: Timo Sachsenberg <timo.sachsenberg@uni-tuebingen.de>

coding convension change

cc0f626

Co-authored-by: Julianus Pfeuffer <pfeuffer@informatik.uni-tuebingen.de>

coding convension change

08538c7

Co-authored-by: Julianus Pfeuffer <pfeuffer@informatik.uni-tuebingen.de>

coding convension change

1f618f9

Co-authored-by: Julianus Pfeuffer <pfeuffer@informatik.uni-tuebingen.de>

ger -> eng

b0ecb9b

Co-authored-by: Julianus Pfeuffer <pfeuffer@informatik.uni-tuebingen.de>

doc change

752c9a6

Co-authored-by: Julianus Pfeuffer <pfeuffer@informatik.uni-tuebingen.de>

Added changes based on code review, added indices to FASTA entry inst…

15cf7cf

…ead of AASequence, changed name to FragmentIndex

fixed racing condition

d03757f

Maxalcer added 3 commits May 11, 2023 12:28

skipped modification part if possible modifications are empty

210723f

moved building the index into seperat function

0062d4b

added modification Index

7923c88

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SearchDatabase Reupload für Codereview #140

SearchDatabase Reupload für Codereview #140

Maxalcer commented May 4, 2023

ksenikrva May 4, 2023

ksenikrva May 4, 2023

ksenikrva May 4, 2023

ksenikrva May 4, 2023

ksenikrva May 4, 2023

ksenikrva May 4, 2023

cbielow May 5, 2023

cbielow May 5, 2023

heie01 May 5, 2023

cbielow May 5, 2023

cbielow May 5, 2023

cbielow May 5, 2023

cbielow May 5, 2023

cbielow May 5, 2023

cbielow May 5, 2023

cbielow May 5, 2023

cbielow May 5, 2023

cbielow May 5, 2023

cbielow May 5, 2023

cbielow May 5, 2023

cbielow May 5, 2023

heie01 May 5, 2023

cbielow May 5, 2023

cbielow May 5, 2023

	///generates sortet vector with all theoretical Fragments for all theoretical Peptides
	///generates sorted vector with all theoretical Fragments for all theoretical Peptides

	size_t missed_cleavages_;
	size_t missed_cleavages_; ///< number of missed cleavages

	auto prec_unit_cond = precursor_mz_tolerance_unit_ == "Da";
	auto prec_unit_cond = precursor_mz_tolerance_unit_ == UNIT_DA;

	vector<string> tolerance_units{"Da", "ppm"};
	vector<string> tolerance_units{UNIT_DA, "ppm"};

	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");

SearchDatabase Reupload für Codereview #140

Are you sure you want to change the base?

SearchDatabase Reupload für Codereview #140

Conversation

Maxalcer commented May 4, 2023

Description

Checklist

How can I get additional information on failed tests during CI

Advanced commands (admins / reviewer only)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment