Feature/osw auto addup spec by singjc · Pull Request #7742 · OpenMS/OpenMS

singjc · 2024-12-06T05:08:21Z

Description

This is 1 of 3 (probably) PRs related to improvements for ion mobility IPF scoring. I'm breaking up my main implementation into smaller PRs to hopefully make the revision easier.

This PR implements the ability to automatically add up spectra across the peak width in retention time. The original implementation extracted spectra at the peak apex, with the option of extracting more spectra across the peak by setting the add_up_spectra_ param to an odd value greater than 1. Now the user can set add_up_spectra_ to -1, which will turn on the automatic computation of add_up_spectra_ based on the percent of the peak width. The general idea behind adding up more spectra, is that it could lead to cleaner / higher signal in the ion mobility extraction.

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

…/var for automatrically determining add_up_spectra based on RT peak width

…penMS#7722

timosachsenberg · 2024-12-06T10:02:15Z

nice addition. I left some comments and added other reviewers.

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

…of spectra to merge

jcharkow

Overall looks good to me.

One thought though is does it make sense to have both the "fixed" and the "percentile" based addition approaches? Would it make more sense to replace the fixed with the percentile approach or are there cases where the fixed approach is better?

I do not think the fixed approach is used in practice, at least for me.

timosachsenberg · 2024-12-09T14:17:53Z

Overall looks good to me.

One thought though is does it make sense to have both the "fixed" and the "percentile" based addition approaches? Would it make more sense to replace the fixed with the percentile approach or are there cases where the fixed approach is better?

I do not think the fixed approach is used in practice, at least for me.

I agree that this would also be my preferred way forward if possible. E.g., only support percentage
with a default parameters (e.g., something along FWHM) and just have reasonable internal boundaries (e.g. a minimum of 1 and a maximum of 5 scans - something that significantly reduces noise but does not increase computation too much).

singjc · 2024-12-09T18:06:22Z

Overall looks good to me.

One thought though is does it make sense to have both the "fixed" and the "percentile" based addition approaches? Would it make more sense to replace the fixed with the percentile approach or are there cases where the fixed approach is better?

I do not think the fixed approach is used in practice, at least for me.

I do agree as well, though I've left the fixed as an option because that's the current approach that is used. But we could make the dynamic merging of spectra the default? Or like Timo suggested, we could have a default where we just clamp between a reasonable range. This would probably be important for cases where we have a really large peak.

timosachsenberg · 2024-12-12T16:43:40Z


    double getRT() const override;

+    double getMetaValue(std::string name) const;


consider using RangeRT

I was looking into applying this, but I think it would require more changes to other parts of OpenSwath. Currently OpenSwath (and some of the other FeatureFinders) stores the leftWidth and rightWidth in the metaValue, which is then used for retrieval at some point. I think the RangeRT would be a good replacement, but I think those changes would maybe be for another PR?

timosachsenberg · 2024-12-12T16:50:15Z


    // find spectrum that is closest to the apex of the peak using binary search
-    std::vector<OpenSwath::SpectrumPtr> spectrum = fetchSpectrumSwath(used_swath_maps, imrmfeature->getRT(), add_up_spectra_, im_range);
+    std::vector<OpenSwath::SpectrumPtr> spectrum = fetchSpectrumSwath(swath_maps, imrmfeature->getRT(), n_merge_spectra, im_range);


could probably be one

…ual amounts on both sides.

timosachsenberg · 2024-12-17T07:37:57Z

Overall looks good to me.
One thought though is does it make sense to have both the "fixed" and the "percentile" based addition approaches? Would it make more sense to replace the fixed with the percentile approach or are there cases where the fixed approach is better?
I do not think the fixed approach is used in practice, at least for me.

I do agree as well, though I've left the fixed as an option because that's the current approach that is used. But we could make the dynamic merging of spectra the default? Or like Timo suggested, we could have a default where we just clamp between a reasonable range. This would probably be important for cases where we have a really large peak.

Yeah, less options = better test coverage, less combinatorics => less things that can get wrong

timosachsenberg · 2024-12-17T07:54:02Z

I think the mock mrmfeature also needs the new method implemented

singjc · 2025-02-24T16:36:04Z

Sorry, been busy with another project. Just wanted to follow up on this, I think I mostly addressed all the comments/suggestions? You mentioned the mock MRMFeature needed the new method implementation? Can you point me to where that is?

timosachsenberg · 2025-02-25T07:59:54Z

This is the error: ../src/tests/class_tests/openms/source/MRMScoring_test.cpp:590:46: error: allocating an object of abstract class type 'OpenSwath::MockMRMFeature'

OpenMS/src/openswathalgo/include/OpenMS/OPENSWATHALGO/DATAACCESS/MockObjects.h

Line 51 in 864b4e5

class OPENSWATHALGO_DLLAPI MockMRMFeature :

Probably an added method is not implemented there.

…S into feature/osw_auto_addup_spec

singjc · 2025-02-25T15:55:36Z

This is the error: ../src/tests/class_tests/openms/source/MRMScoring_test.cpp:590:46: error: allocating an object of abstract class type 'OpenSwath::MockMRMFeature'

OpenMS/src/openswathalgo/include/OpenMS/OPENSWATHALGO/DATAACCESS/MockObjects.h

Line 51 in 864b4e5

class OPENSWATHALGO_DLLAPI MockMRMFeature :

Probably an added method is not implemented there.

Ahh kk, I updated the MockObjects. Should work now, passed locally.

timosachsenberg · 2025-02-25T17:41:36Z

I think these interfaces would profit a lot from some documentation in the future.
e.g. such that things like the difference between:
virtual void getIntensity(std::vector& intens) const = 0;
virtual float getIntensity() const = 0;

for what virtual double getMetaValue(std::string name) const = 0; is used (and ideally which ones are currently queried)

singjc · 2025-02-27T14:31:16Z

All tests pass, can we merge?

singjc added 5 commits December 5, 2024 23:15

add: getMetaValue method to IMRMFeature interface

b151b83

add: getMetaValue method to MRMFeatureOpenMS

1f4e89a

update,add: add_up_spectra arg doc and add use_percent_peak_width arg…

0c3cb9c

…/var for automatrically determining add_up_spectra based on RT peak width

chore: remove sonar swath map check for calculateDIAIdScores. See PR O…

a6f9c3c

…penMS#7722

add: auto add up spectra logic to DIA ipf scoring

6f5e18b

timosachsenberg requested changes Dec 6, 2024

View reviewed changes

timosachsenberg reviewed Dec 6, 2024

View reviewed changes

Comment thread src/openms/source/ANALYSIS/OPENSWATH/OpenSwathScoring.cpp Outdated

timosachsenberg requested review from cbielow and jcharkow December 6, 2024 10:01

singjc and others added 4 commits December 6, 2024 09:35

Update src/openms/include/OpenMS/ANALYSIS/OPENSWATH/OpenSwathScoring.h

d7d32c1

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

Update src/openms/source/ANALYSIS/OPENSWATH/MRMFeatureFinderScoring.cpp

f8156e8

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

update: use_percent_peak_width to merge_spectra_by_peak_width_fraction_

60249c6

add: SpectrumMergeMethodType enum to specify fixed or dynamic number …

1dfa22d

…of spectra to merge

timosachsenberg reviewed Dec 9, 2024

View reviewed changes

Comment thread src/openms/source/ANALYSIS/OPENSWATH/MRMFeatureFinderScoring.cpp Outdated

timosachsenberg reviewed Dec 9, 2024

View reviewed changes

Comment thread src/openms/source/ANALYSIS/OPENSWATH/OpenSwathScoring.cpp Outdated

jcharkow approved these changes Dec 9, 2024

View reviewed changes

timosachsenberg reviewed Dec 12, 2024

View reviewed changes

singjc added 2 commits December 13, 2024 13:47

chore,doc: make add up spec doc more clearer, to add up spectra by eq…

dec8c7b

…ual amounts on both sides.

fix,clean: remove hack to enfore odd number for n_merge_spectra

48e2dcf

singjc requested a review from timosachsenberg December 13, 2024 19:09

Merge branch 'develop' into feature/osw_auto_addup_spec

49c4b9d

Merge branch 'develop' into feature/osw_auto_addup_spec

c6d071b

singjc added 2 commits February 25, 2025 10:54

add,fix: getMetaValue to MockObjects

ca39d09

Merge branch 'feature/osw_auto_addup_spec' of github.com:singjc/OpenM…

2311164

…S into feature/osw_auto_addup_spec

fix,update: OpenSwathScoring test, with updated params in intializer

c2403e7

timosachsenberg reviewed Feb 25, 2025

View reviewed changes

timosachsenberg approved these changes Feb 25, 2025

View reviewed changes

Merge branch 'develop' into feature/osw_auto_addup_spec

634b75b

timosachsenberg merged commit f9610b4 into OpenMS:develop Feb 27, 2025


		double getRT() const override;

		double getMetaValue(std::string name) const;

Uh oh!

Uh oh!

Conversation

singjc commented Dec 6, 2024

Description

Checklist

How can I get additional information on failed tests during CI

Advanced commands (admins / reviewer only)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timosachsenberg commented Dec 6, 2024

Uh oh!

Uh oh!

Uh oh!

jcharkow left a comment

Choose a reason for hiding this comment

Uh oh!

timosachsenberg commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

singjc commented Dec 9, 2024

Uh oh!

timosachsenberg Dec 12, 2024 • 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!

singjc Dec 13, 2024

Choose a reason for hiding this comment

Uh oh!

timosachsenberg Dec 12, 2024

Choose a reason for hiding this comment

Uh oh!

timosachsenberg commented Dec 17, 2024

Uh oh!

timosachsenberg commented Dec 17, 2024

Uh oh!

singjc commented Feb 24, 2025

Uh oh!

timosachsenberg commented Feb 25, 2025

Uh oh!

singjc commented Feb 25, 2025

Uh oh!

timosachsenberg Feb 25, 2025

Choose a reason for hiding this comment

Uh oh!

singjc commented Feb 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

timosachsenberg commented Dec 9, 2024 •

edited

Loading

timosachsenberg Dec 12, 2024 •

edited

Loading