Optimization in OpenSwathWorkflow #92

xchen98 · 2020-05-11T07:07:20Z

No description provided.

xchen98 · 2020-05-12T13:52:19Z

1.Part: SignalToNoiseEstimator
It coud be found that the type pf data, map<PeakType, doule>, was used in SignalToNoiseEstimator of Tool OpenSwathWorkflow,in which the values corresponded to the iteration of map are storaged. The stored values are synchronously updated contiously with the iteration, using the funtion named getSignalToNoise. However, vector could be more efficient when processing large amounts of data. Thus,vectors are applied here.From the diagram shown below, it could be easily found that the processing speed differs a lot while using vector and map.(left: map | right: vector)
runtime of OpenSwathWorkflow with map: 18:23.228 s
runtime of OpenSwathWorkflow with vector: 15:17.435s

taranehstrunk · 2020-05-15T10:14:04Z

src/openms/include/OpenMS/FILTERING/NOISEESTIMATION/SignalToNoiseEstimatorMeanIterative.h

@@ -39,6 +39,7 @@
 #include <OpenMS/CONCEPT/Exception.h>
 #include <OpenMS/DATASTRUCTURES/ListUtils.h>
 #include <vector>
+#include <algorithm>


in other files the specific reason for a (non-obvious) include was commented. E.g. #include <algorithm> // for std::max_element

I'll edit it later ,thanks for your commend :)

taranehstrunk · 2020-05-15T10:45:45Z

src/openms/include/OpenMS/FILTERING/NOISEESTIMATION/SignalToNoiseEstimatorMedian.h

+//        ++windows_overall;
+//        ++run;
+//      }
+      int windows_overall = c.size();


int windows_overall is only used once here - neccessary?

It can be used directly in the function . If we used it, it can makes code more clear and understandable.

taranehstrunk · 2020-05-15T10:48:52Z

src/openms/include/OpenMS/FILTERING/NOISEESTIMATION/SignalToNoiseEstimatorMeanIterative.h

@@ -370,7 +376,7 @@ namespace OpenMS
        }

        // store result
-        stn_estimates_[*window_pos_center] = (*window_pos_center).getIntensity() / noise;
+        stn_estimates_[window_count] = ((*window_pos_center).getIntensity() / noise);


Why has this line been changed? I don't see a change in functionality.

The type of stn_estimates_ is map, map<PeakType, double>, so I used the iteration(*window_pos_center) to assign it.
But the type of stn_estimates now is vector, so I used a index(int window_count) instead.

taranehstrunk · 2020-05-15T11:12:40Z

src/openms/include/OpenMS/FILTERING/NOISEESTIMATION/SignalToNoiseEstimator.h

-    virtual double getSignalToNoise(const PeakType & data_point)
-    {
-      if (!is_result_valid_)
+      virtual double getSignalToNoise(const Size index)


Maybe call const Size index something that is related to data_point. Index doesn't really state anything about the variable's functionality.
Also it looks like there's an additional tab before virtual double...

You can see the last answer of mine.

taranehstrunk · 2020-05-15T11:21:54Z

src/openms/include/OpenMS/FILTERING/NOISEESTIMATION/SignalToNoiseEstimator.h

@@ -106,46 +109,58 @@ namespace OpenMS



If we stick to the rule of six, don't we have to add move constructor and move assignment operator?

I have already added them (c with type Container).

taranehstrunk · 2020-05-15T12:27:14Z

src/openms/include/OpenMS/FILTERING/NOISEESTIMATION/SignalToNoiseEstimatorMedian.h

@@ -275,7 +282,6 @@ namespace OpenMS
        std::cerr << "TODO SignalToNoiseEstimatorMedian: the max_intensity_ value should be positive! " << max_intensity_ << std::endl;
        return;
      }
-


Deleted empty line

taranehstrunk · 2020-05-15T12:34:03Z

src/openms/source/TRANSFORMATIONS/RAW2PEAK/PeakPickerCWT.cpp

@@ -1222,7 +1222,7 @@ namespace OpenMS
             && (shape.getFWHM() >= fwhm_bound_)
             && (shape.getFWHM() <= fwhm_upper_bound))
          {
-            shape.signal_to_noise = sne.getSignalToNoise(area.max);
+            shape.signal_to_noise = sne.getSignalToNoise((Size)distance(raw_peak_array.begin(),area.max));
            peak_shapes.push_back(shape);


Is it possible to use emplace_back instead?

It is possible.

taranehstrunk · 2020-05-15T12:35:05Z

src/openms/source/TRANSFORMATIONS/RAW2PEAK/PeakPickerCWT.cpp

@@ -1179,7 +1179,7 @@ namespace OpenMS
      {
        // if the signal to noise ratio at the max position is too small
        // the peak isn't considered
-        if ((area.max != it_pick_end) && (sne.getSignalToNoise(area.max) < signal_to_noise_))
+        if ((area.max != it_pick_end) && (sne.getSignalToNoise((Size)distance(raw_peak_array.begin(),area.max)) < signal_to_noise_))


Could you use it_pick_begin ?

yes, I can, but I ignored it before.

taranehstrunk · 2020-05-15T12:42:25Z

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

-END_SECTION
-
-START_SECTION((virtual double getSignalToNoise(const PeakType &data_point)))
+START_SECTION((virtual double getSignalToNoise(const Size index)))
  // hard to do without implementing computeSTN_ properly


What does "properly" mean? Maybe it can be tested now?

Suggested change

// hard to do without implementing computeSTN_ properly

// hard to do without implementing computeSTN_ properly

It is just a comment. Nothing between START_SECTION and END_SECTION can be tested.

taranehstrunk

In general I would suggest to delete all the lines that have now been replaced instead of just commenting them out.

cbielow · 2020-05-25T08:46:41Z

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


 namespace OpenMS
 {
  /**
   * @brief Pre-calculate isotope distributions for interesting mass ranges
   */
-  class OPENMS_DLLAPI IsotopeDistributionCache
+  class OPENMS_DLLAPI IsotopeDistributionCache:
+          public FeatureFinderAlgorithmPickedHelperStructs


why inherit from FeatureFinderAlgorithmPickedHelperStructs????

sorry, I forget to delete it.

cbielow · 2020-05-25T08:47:36Z

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


 private:
    /// Vector of pre-calculated isotope distributions for several mass windows
    std::vector<TheoreticalIsotopePattern> isotope_distributions_;

+    std::vector<IsotopeDistribution> intensity_;


intensity_ is not a good name. Its a distribution...

We'll call the member distribution_cache_ instead.

cbielow · 2020-05-25T08:50:58Z

src/openms/source/ANALYSIS/OPENSWATH/DIAScoring.cpp

@@ -394,37 +400,13 @@ namespace OpenMS
      // create the theoretical distribution from the sum formula
      EmpiricalFormula empf(sum_formula);
      isotope_dist = empf.getIsotopeDistribution(CoarseIsotopePatternGenerator(dia_nr_isotopes_));
+      iso_.renormalize(isotopes, isotope_dist);


here, the CoarseIsotopePatternGenerator is still called.
Are you sure this branch is not taken?
In any case, replace it with Cache...

cbielow · 2020-05-25T08:52:05Z

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

  {
-    Size num_isotopes = std::ceil(max_mass / mass_window_width) + 1;
+    mass_window_width_ = 20.0;


move initialization to initializer list (as done for mass_window_width_(mass_window_width) before)

cbielow · 2020-05-25T08:55:11Z

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

+
+  }
+
+  void IsotopeDistributionCache::renormalize(


can isotope_dist be a const param? You are only reading from it??

cbielow · 2020-05-25T08:55:46Z

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

+  }
+
+
+  void IsotopeDistributionCache::calculateisotopeDistribution(OpenMS::Size num_begin)


calculateisotopeDistribution is a bit unspecific.
You are effectively extending the cache?

cbielow · 2020-05-25T08:58:20Z

src/openms/include/OpenMS/ANALYSIS/OPENSWATH/DIAScoring.h

@@ -215,6 +216,8 @@ namespace OpenMS
    bool dia_extraction_ppm_;
    bool dia_centroided_;

+    IsotopeDistributionCache iso_;


can you call this iso_cache_ or something which reflects its purpose?

cbielow · 2020-05-25T09:24:51Z

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

    if (index >= isotope_distributions_.size())
    {
-      throw Exception::InvalidValue(__FILE__, __LINE__, OPENMS_PRETTY_FUNCTION, "IsotopeDistribution not precalculated. Maximum allowed index is " + String(isotope_distributions_.size()), String(index));
+      Size size = isotope_distributions_.size();


this code should move into calculateisotopeDistribution.
simply pass the new maximum index which you need to have filled.
The rest should happen in the function.
Avoids code duplication (see b2b8728#diff-8d025ab465e7de035c1eae4c0d2440b9R145)

taranehstrunk reviewed May 15, 2020

View reviewed changes

xchen98 force-pushed the chen branch from 20a8725 to f99300d Compare May 18, 2020 10:27

xchen98 closed this May 18, 2020

xchen98 force-pushed the chen branch from c3cc8b6 to 0f05822 Compare May 18, 2020 10:42

prepareLine

df31ffb

xchen98 reopened this May 20, 2020

SignalToNoiseEstimator

22251be

xchen98 changed the title ~~Optimization in SignalToNoiseEstimator of OpenSwathWorkflow~~ Optimization in OpenSwathWorkflow May 20, 2020

Precomputation for DIAScoring and DIAHelper

b2b8728

cbielow reviewed May 25, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimization in OpenSwathWorkflow #92

Optimization in OpenSwathWorkflow #92

xchen98 commented May 11, 2020

xchen98 commented May 12, 2020 •

edited

Loading

taranehstrunk May 15, 2020

xchen98 May 15, 2020 •

edited

Loading

taranehstrunk May 15, 2020

xchen98 May 15, 2020

taranehstrunk May 15, 2020

xchen98 May 15, 2020

taranehstrunk May 15, 2020 •

edited

Loading

xchen98 May 15, 2020

taranehstrunk May 15, 2020

xchen98 May 15, 2020

taranehstrunk May 15, 2020

taranehstrunk May 15, 2020

xchen98 May 15, 2020

taranehstrunk May 15, 2020

xchen98 May 15, 2020

taranehstrunk May 15, 2020

xchen98 May 15, 2020

taranehstrunk left a comment

cbielow May 25, 2020

xchen98 May 25, 2020

cbielow May 25, 2020

taranehstrunk May 25, 2020

cbielow May 25, 2020

cbielow May 25, 2020

cbielow May 25, 2020

cbielow May 25, 2020

cbielow May 25, 2020

cbielow May 25, 2020

	// hard to do without implementing computeSTN_ properly
	// hard to do without implementing computeSTN_ properly

		}


		void IsotopeDistributionCache::calculateisotopeDistribution(OpenMS::Size num_begin)

Optimization in OpenSwathWorkflow #92

Are you sure you want to change the base?

Optimization in OpenSwathWorkflow #92

Conversation

xchen98 commented May 11, 2020

xchen98 commented May 12, 2020 • edited Loading

Choose a reason for hiding this comment

xchen98 May 15, 2020 • edited Loading

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

taranehstrunk May 15, 2020 • edited Loading

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

taranehstrunk left a 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

xchen98 commented May 12, 2020 •

edited

Loading

xchen98 May 15, 2020 •

edited

Loading

taranehstrunk May 15, 2020 •

edited

Loading