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

Integrate MALDIquant's baseline estimation #119 #120

Merged
merged 51 commits into from
Dec 28, 2023
Merged

Integrate MALDIquant's baseline estimation #119 #120

merged 51 commits into from
Dec 28, 2023

Conversation

lgatto
Copy link
Member

@lgatto lgatto commented Dec 26, 2023

Addresses #119

@lgatto
Copy link
Member Author

lgatto commented Dec 26, 2023

The GHA fails - I have tried to fiddle with it, so just ignore the workflow file for now. (Or any advise welcome)

@lgatto
Copy link
Member Author

lgatto commented Dec 26, 2023

General points to consider for this review:

  1. Example data used in ?estimateBaseline comes from MALDIquant, which is thus now in suggest, rather than part of the package itself (size is about 0.5 Mb)
  2. There's one single estimateBaseline() function with a method argument. We could also provide individual estimateBaselineSNIP(), estimateBaselineTopHat(), esimateBaselineConvexHull() and estimateBaselineMedian().

@lgatto
Copy link
Member Author

lgatto commented Dec 27, 2023

In addition to using simulated example data, I also export the individual estimateBaseline[Snip|TopHat|ConvexHub|Media]() functions. I think this makes arguments explicit, and clarifies the manual page. The main estimateBaseline() function is still available, so as to remain backward compatible with the signature in MALDIquant.

Copy link
Member

@sgibb sgibb left a comment

Choose a reason for hiding this comment

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

Thanks @lgatto for integrating the baseline estimation algorithms!

I just changed some values for the iteration/half window sizes to demonstrate the difference in the simulated data.

R/estimateBaseline.R Outdated Show resolved Hide resolved
R/estimateBaseline.R Outdated Show resolved Hide resolved
R/estimateBaseline.R Outdated Show resolved Hide resolved
R/estimateBaseline.R Outdated Show resolved Hide resolved
R/estimateBaseline.R Outdated Show resolved Hide resolved
R/estimateBaseline.R Outdated Show resolved Hide resolved
lgatto and others added 6 commits December 28, 2023 09:43
Co-authored-by: Sebastian Gibb <mail@sebastiangibb.de>
Co-authored-by: Sebastian Gibb <mail@sebastiangibb.de>
Co-authored-by: Sebastian Gibb <mail@sebastiangibb.de>
Co-authored-by: Sebastian Gibb <mail@sebastiangibb.de>
Co-authored-by: Sebastian Gibb <mail@sebastiangibb.de>
Co-authored-by: Sebastian Gibb <mail@sebastiangibb.de>
@lgatto
Copy link
Member Author

lgatto commented Dec 28, 2023

Thank you @sgibb!

@lgatto lgatto merged commit 42373a3 into main Dec 28, 2023
0 of 4 checks passed
@lgatto lgatto deleted the lgmain branch March 18, 2024 18:05
@lgatto lgatto restored the lgmain branch March 18, 2024 18:05
@lgatto lgatto deleted the lgmain branch March 18, 2024 18:07
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.

2 participants