Skip to content
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

fix imputation bug on single peak mass traces #5702

Merged
merged 1 commit into from
Dec 13, 2021

Conversation

timosachsenberg
Copy link
Contributor

@timosachsenberg timosachsenberg commented Dec 13, 2021

Description

Might fix: #5678

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:

If your PR is failing you can check out

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

@jpfeuffer
Copy link
Contributor

To be honest, after looking into the code, I am not sure if this will solve the problem:

defaults_.setValue("peak_width", -1.0, "Force a certain minimal peak_width on the data (e.g. extend the peak at least by this amount on both sides) in seconds. -1 turns this feature off.");

It does not stop the picking or invalidates the transition group. It just makes sure that it at least picks up a peaks in a certain range around the "center" peak.

|| (peak_width_ > 0.0 && std::fabs(chromatogram[min_i - k].getRT() - central_peak_rt) < peak_width_))

Did you check this?

@timosachsenberg
Copy link
Contributor Author

I made the change based on @hroest comment:
Also currently MRMTransitionGroupFinder::createMRMFeature uses a min_peak_width_ so in principle this can be prevent there by setting this to non-zero

@jpfeuffer
Copy link
Contributor

Ah sorry, didnt check the header. There is another usage of the member:
https://github.com/OpenMS/OpenMS/blob/develop/src/openms/include/OpenMS/ANALYSIS/OPENSWATH/MRMTransitionGroupPicker.h#L277

Ok should be fine

@timosachsenberg
Copy link
Contributor Author

ok thanks for checking.
Maybe one could even be stricter using the information here:
https://github.com/OpenMS/OpenMS/blob/develop/src/openms/include/OpenMS/ANALYSIS/OPENSWATH/MRMTransitionGroupPicker.h#L237-L238
to check if we have e.g. at least 3 non-zero peaks in the chromatogram - or similar

@timosachsenberg timosachsenberg merged commit 72ff959 into develop Dec 13, 2021
@timosachsenberg timosachsenberg deleted the feature/fix_single_peak_imputation branch December 13, 2021 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question: ElutionModelFitter imputation
2 participants