Push MTD by t-aretz · Pull Request #143 · cbielow/OpenMS

t-aretz · 2023-05-05T07:41:06Z

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

maxvk79 · 2023-05-05T08:14:03Z

src/openms/include/OpenMS/FILTERING/DATAREDUCTION/MassTraceDetection.h

+        // calc_left_border_
+        Size calc_left_border_(Size peak_index_in_apices_vec, double mass_error_ppm_, const PeakMap& input_exp, const std::vector<Apex>& apices_vec);
+
        // parameter stuff


what does stuff mean?

Suggestion: more precise description

maxvk79 · 2023-05-05T08:15:09Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

+#include <omp.h>
+// #include <parallel/algorithm>
+// #include <ctime>
+#include <chrono>
+
+#include <OpenMS/MATH/MISC/MathFunctions.h>


sort includes lexicographically

maxvk79 · 2023-05-05T08:16:32Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp


    void MassTraceDetection::run(const PeakMap& input_exp, std::vector<MassTrace>& found_masstraces, const Size max_traces)
    {
+


why blank line?

maxvk79 · 2023-05-05T08:24:47Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

      return;
    } // end of MassTraceDetection::run

+    double MassTraceDetection::find_offset_(Size peak_index_in_apices_vec, double mass_error_ppm_, const PeakMap& input_exp, const std::vector<Apex>& apices_vec)


Suggested change

double MassTraceDetection::find_offset_(Size peak_index_in_apices_vec, double mass_error_ppm_, const PeakMap& input_exp, const std::vector<Apex>& apices_vec)

double MassTraceDetection::find_offset_(Size peak_index_in_apices_vec, double mass_error_ppm_, const PeakMap& intput_map, const std::vector<Apex>& apices_vec)

input_exp may not be concise name.

maxvk79 · 2023-05-05T08:26:54Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

+    {
+      double centroid_mz = input_exp[apices_vec[peak_index_in_apices_vec].scan_idx][apices_vec[peak_index_in_apices_vec].peak_idx].getMZ(); //create function
+      // double ftl_sd((centroid_mz / 1e6) * mass_error_ppm_);                      
+      double ftl_sd = Math::ppmToMass(mass_error_ppm_,centroid_mz);  // mit formel ergänzen header ppm to dalton


what is the name "ftl_sd" supposed to mean?
more precise naming

maxvk79 · 2023-05-05T08:33:28Z

src/openms/include/OpenMS/FILTERING/DATAREDUCTION/MassTraceDetection.h

coding convention! in several function names: instead of foo_bar() do fooBar()

double find_offset_(Size peak_index_in_apices_vec, double mass_error_ppm_, const PeakMap& input_exp, const std::vector& apices_vec);

maxvk79 · 2023-05-05T08:42:42Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

+      Size j{};
+      while (input_exp[apices_vec[peak_index_in_apices_vec + j].scan_idx][apices_vec[peak_index_in_apices_vec + j].peak_idx].getMZ() <= right_bound) 
+      { 
+        if(peak_index_in_apices_vec + j >= apices_vec.size()) break;


Suggested change

if(peak_index_in_apices_vec + j >= apices_vec.size()) break;

if(peak_index_in_apices_vec + j >= apices_vec.size())

{

break;

}

maxvk79 · 2023-05-05T08:50:37Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

+
+    Size MassTraceDetection::calc_right_border_(Size peak_index_in_apices_vec, double mass_error_ppm_, const PeakMap& input_exp, const std::vector<Apex>& apices_vec)
+    {
+      double right_bound = (input_exp[apices_vec[peak_index_in_apices_vec].scan_idx][apices_vec[peak_index_in_apices_vec].peak_idx].getMZ()) + (3 /* border gerade 3 mal so groß wie "noetig" */ * find_offset_(peak_index_in_apices_vec, mass_error_ppm_, input_exp, apices_vec));


Suggested change

double right_bound = (input_exp[apices_vec[peak_index_in_apices_vec].scan_idx][apices_vec[peak_index_in_apices_vec].peak_idx].getMZ()) + (3 /* border gerade 3 mal so groß wie "noetig" */ * find_offset_(peak_index_in_apices_vec, mass_error_ppm_, input_exp, apices_vec));

double tmp_mz = input_exp[apices_vec[peak_index_in_apices_vec].scan_idx][apices_vec[peak_index_in_apices_vec].peak_idx].getMZ();

double right_bound = tmp_mz + 3 * find_offset_(peak_index_in_apices_vec, mass_error_ppm_, input_exp, apices_vec); //border gerade 3 mal so groß wie "noetig"

maxvk79 · 2023-05-05T08:56:19Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

+      return peak_index_in_apices_vec + j;
+    }
+
+    Size MassTraceDetection::calc_left_border_(Size peak_index_in_apices_vec, double mass_error_ppm_, const PeakMap& input_exp, const std::vector<Apex>& apices_vec)


same as in right border :)

maxvk79 · 2023-05-05T08:58:51Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

-      for (auto m_it = chrom_apices.crbegin(); m_it != chrom_apices.crend(); ++m_it)
+
+      // Size binnumber{1};
+      Size binnumber = omp_get_max_threads();


Suggested change

Size binnumber = omp_get_max_threads();

Size bin_number = omp_get_max_threads();

maxvk79 · 2023-05-05T09:05:41Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

-        double prev_counter(apex_peak.getIntensity() * apex_peak.getMZ());
-        double prev_denom(apex_peak.getIntensity());
+      #pragma omp parallel for num_threads(binnumber)
+      for (Size i = 0; i < binnumber; ++i)


Suggested change

for (Size i = 0; i < binnumber; ++i)

for(Size i = 0; i < binnumber; ++i)

maxvk79 · 2023-05-05T09:06:10Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

+                   {
+                     return a.intensity > b.intensity;
+                   });
+        for (auto m_it = chrom_apices_2.cbegin(); m_it != chrom_apices_2.cend(); ++m_it) // iterate reverse from high intensity to low intensity


Suggested change

for (auto m_it = chrom_apices_2.cbegin(); m_it != chrom_apices_2.cend(); ++m_it) // iterate reverse from high intensity to low intensity

for(auto m_it = chrom_apices_2.cbegin(); m_it != chrom_apices_2.cend(); ++m_it) // iterate reverse from high intensity to low intensity

maxvk79 · 2023-05-05T09:07:32Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

+        std::sort(chrom_apices_2.begin(), chrom_apices_2.end(),   // sort by intensity 
+        [](const Apex & a, const Apex & b) -> bool
+                   {
+                     return a.intensity > b.intensity;
+                   });


Suggested change

std::sort(chrom_apices_2.begin(), chrom_apices_2.end(), // sort by intensity

[](const Apex & a, const Apex & b) -> bool

{

return a.intensity > b.intensity;

});

std::sort(chrom_apices_2.begin(), chrom_apices_2.end(), // sort by intensity

[](const Apex & a, const Apex & b) -> bool

{

return a.intensity > b.intensity;

});

maxvk79 · 2023-05-05T09:12:18Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

-          // Step 2.1 MOVE DOWN in RT dim
-          // *********************************************************** //
-          if ((trace_down_idx > 0) && toggle_down)
+          std::list<PeakType> current_trace;


use std:.vector if possible

maxvk79 · 2023-05-05T09:12:39Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

+          std::list<PeakType> current_trace;
+          double startint = work_exp[apex_scan_idx][apex_peak_idx].getIntensity();
+          current_trace.push_back(apex_peak);
+          std::vector<double> fwhms_mz; // peak-FWHM meta values of collected peaks


what does the s mean in fwhms_mz?

maxvk79 · 2023-05-05T09:17:17Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

+          double prev_counter(apex_peak.getIntensity() * apex_peak.getMZ());
+          double prev_denom(apex_peak.getIntensity());
+
+          updateIterativeWeightedMeanMZ(apex_peak.getMZ(), apex_peak.getIntensity(), centroid_mz, prev_counter, prev_denom);


Suggested change

updateIterativeWeightedMeanMZ(apex_peak.getMZ(), apex_peak.getIntensity(), centroid_mz, prev_counter, prev_denom);

updateIterativeWeightedMeanMZ(centroid_mz, prev_denom, centroid_mz, prev_counter, prev_denom);

why multiple inputs of the same thing

maxvk79 · 2023-05-05T09:23:38Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

+          while (((trace_down_idx > 0) && toggle_down) ||
+                ((trace_up_idx < work_exp.size() - 1) && toggle_up)
+                  )


Suggested change

while (((trace_down_idx > 0) && toggle_down) ||

((trace_up_idx < work_exp.size() - 1) && toggle_up)

)

while (trace_down_idx > 0 && toggle_down || trace_up_idx < work_exp.size() - 1 && toggle_up)

maxvk79 · 2023-05-05T09:26:41Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

+            // *********************************************************** //
+            // Step 2.1 MOVE DOWN in RT dim
+            // *********************************************************** //
+            if ((trace_down_idx > 0) && toggle_down)


Suggested change

if ((trace_down_idx > 0) && toggle_down)

if (trace_down_idx > 0 && toggle_down)

maxvk79 · 2023-05-05T09:29:12Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

+                if ((next_down_peak_mz <= right_bound) &&
+                    (next_down_peak_mz >= left_bound) &&
+                    !peak_visited_1[spec_offsets[trace_down_idx - 1] + next_down_peak_idx]
+                        )


Suggested change

if ((next_down_peak_mz <= right_bound) &&

(next_down_peak_mz >= left_bound) &&

!peak_visited_1[spec_offsets[trace_down_idx - 1] + next_down_peak_idx]

)

if (next_down_peak_mz <= right_bound &&

next_down_peak_mz >= left_bound &&

!peak_visited_1[spec_offsets[trace_down_idx - 1] + next_down_peak_idx])

maxvk79 · 2023-05-05T09:31:11Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

-                // Update the m/z mean of the current trace as we added a new peak
-                updateIterativeWeightedMeanMZ(next_down_peak_mz, next_down_peak_int, centroid_mz, prev_counter, prev_denom);
-                gathered_idx.emplace_back(trace_down_idx - 1, next_down_peak_idx);
+                  if(next_down_peak_int > startint)


is this for debugging?

maxvk79 · 2023-05-05T09:34:30Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

+            // *********************************************************** //
+            // Step 2.2 MOVE UP in RT dim
+            // *********************************************************** //
+            if ((trace_up_idx < work_exp.size() - 1) && toggle_up)


Suggested change

if ((trace_up_idx < work_exp.size() - 1) && toggle_up)

if (trace_up_idx < work_exp.size() - 1 && toggle_up)

maxvk79 · 2023-05-05T09:36:25Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

+                if ((next_up_peak_mz <= right_bound) &&
+                    (next_up_peak_mz >= left_bound) &&
+                    !peak_visited_1[spec_offsets[trace_up_idx + 1] + next_up_peak_idx])


Suggested change

if ((next_up_peak_mz <= right_bound) &&

(next_up_peak_mz >= left_bound) &&

!peak_visited_1[spec_offsets[trace_up_idx + 1] + next_up_peak_idx])

if (next_up_peak_mz <= right_bound &&

next_up_peak_mz >= left_bound &&

!peak_visited_1[spec_offsets[trace_up_idx + 1] + next_up_peak_idx])

maxvk79 · 2023-05-05T09:37:38Z

src/openms/source/FILTERING/DATAREDUCTION/MassTraceDetection.cpp

+                  // Update the m/z variance dynamically
+                  if (reestimate_mt_sd_)           //  && (up_hitting_peak+1 > min_flank_scans))
+                  {
+                    // if (ftl_t > min_fwhm_scans)
+                    {
+                      updateWeightedSDEstimateRobust(next_peak, centroid_mz, ftl_sd, intensity_so_far);
+                    }
+                  }


Suggested change

// Update the m/z variance dynamically

if (reestimate_mt_sd_) // && (up_hitting_peak+1 > min_flank_scans))

{

// if (ftl_t > min_fwhm_scans)

{

updateWeightedSDEstimateRobust(next_peak, centroid_mz, ftl_sd, intensity_so_far);

}

}

// Update the m/z variance dynamically

if (reestimate_mt_sd_)// && (up_hitting_peak+1 > min_flank_scans))

{

updateWeightedSDEstimateRobust(next_peak, centroid_mz, ftl_sd, intensity_so_far);

}

maxvk79

Mainly checked for syntax and coding convention

Push MTD

2c28332

maxvk79 reviewed May 5, 2023

View reviewed changes

t-aretz added 5 commits May 15, 2023 15:51

Deque for MassTrace

54fcc16

Works but not pretty or finished, but works

97ab6d2

Sort In Place, no copies, less memory

314ee53

First Steps in queue try

3704886

works but not really parallel, why?

b3d8a9c

t-aretz added 18 commits May 18, 2023 10:15

Different outcome with different threads and slow

a9dbb3a

gitignore add smaple file

4f03fd6

Another Approach and search_traces function

82a6c42

noch mit bug, fehler noch nicht gefunden

fbe1d4f

Some changes

e4fc8ab

Searches in Funtions, for trying pragma omp tasks

2a9a523

Intensity Version

b5711d4

Same result with different threads

dc98474

Other while loop break

0ad144d

Fix small bug, added (add_to_peaks) critical

f9b4c36

Deadlock ?

17d5809

with peak_visited right

306f607

one thread too many traces, more threads deadlock

5db5572

Changed Interface

6b5d2cc

more threads lead to memory errors

c434153

Läuft, zu viele Traces noch

6e676a8

klappt

7d5e4e8

klappt

43c748a


		void MassTraceDetection::run(const PeakMap& input_exp, std::vector<MassTrace>& found_masstraces, const Size max_traces)
		{

	double MassTraceDetection::find_offset_(Size peak_index_in_apices_vec, double mass_error_ppm_, const PeakMap& input_exp, const std::vector<Apex>& apices_vec)
	double MassTraceDetection::find_offset_(Size peak_index_in_apices_vec, double mass_error_ppm_, const PeakMap& intput_map, const std::vector<Apex>& apices_vec)

	double right_bound = (input_exp[apices_vec[peak_index_in_apices_vec].scan_idx][apices_vec[peak_index_in_apices_vec].peak_idx].getMZ()) + (3 /* border gerade 3 mal so groß wie "noetig" / find_offset_(peak_index_in_apices_vec, mass_error_ppm_, input_exp, apices_vec));
	double tmp_mz = input_exp[apices_vec[peak_index_in_apices_vec].scan_idx][apices_vec[peak_index_in_apices_vec].peak_idx].getMZ();
	double right_bound = tmp_mz + 3 * find_offset_(peak_index_in_apices_vec, mass_error_ppm_, input_exp, apices_vec); //border gerade 3 mal so groß wie "noetig"

	Size binnumber = omp_get_max_threads();
	Size bin_number = omp_get_max_threads();

	for (Size i = 0; i < binnumber; ++i)
	for(Size i = 0; i < binnumber; ++i)

	for (auto m_it = chrom_apices_2.cbegin(); m_it != chrom_apices_2.cend(); ++m_it) // iterate reverse from high intensity to low intensity
	for(auto m_it = chrom_apices_2.cbegin(); m_it != chrom_apices_2.cend(); ++m_it) // iterate reverse from high intensity to low intensity

	updateIterativeWeightedMeanMZ(apex_peak.getMZ(), apex_peak.getIntensity(), centroid_mz, prev_counter, prev_denom);
	updateIterativeWeightedMeanMZ(centroid_mz, prev_denom, centroid_mz, prev_counter, prev_denom);

	if ((trace_down_idx > 0) && toggle_down)
	if (trace_down_idx > 0 && toggle_down)

	if ((trace_up_idx < work_exp.size() - 1) && toggle_up)
	if (trace_up_idx < work_exp.size() - 1 && toggle_up)

Conversation

t-aretz commented May 5, 2023

Description

Checklist

How can I get additional information on failed tests during CI

Advanced commands (admins / reviewer only)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxvk79 May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxvk79 May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxvk79 May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxvk79 May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxvk79 May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxvk79 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

maxvk79 May 5, 2023 •

edited

Loading

maxvk79 May 5, 2023 •

edited

Loading

maxvk79 May 5, 2023 •

edited

Loading

maxvk79 May 5, 2023 •

edited

Loading

maxvk79 May 5, 2023 •

edited

Loading