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

Added extended findPeaks function #50

Merged
merged 9 commits into from
Sep 26, 2016
Merged

Conversation

Treutler
Copy link
Contributor

No description provided.

@@ -305,7 +305,8 @@ setMethod("findPeaks.matchedFilter", "xcmsRaw",
setMethod("findPeaks.centWave", "xcmsRaw", function(object, ppm=25, peakwidth=c(20,50), snthresh=10,
prefilter=c(3,100), mzCenterFun="wMean", integrate=1, mzdiff=-0.001,
fitgauss=FALSE, scanrange= numeric(), noise=0, ## noise.local=TRUE,
sleep=0, verbose.columns=FALSE, ROI.list=list()) {
sleep=0, verbose.columns=FALSE, ROI.list=list(),
perform1stBaselineCheck=TRUE, roiScales=NULL) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some comments here:

  1. Is there a need to add these new parameters at all? They are not needed by your new findPeaks method.
  2. Rename perform1stBaselineCheck to firstBaselineCheck.
  3. You should perform some input argument checking, i.e. that length roiScales matches length of ROI.list and that roiScales contains numeric values.
  4. Most importantly however: document the new arguments in the corresponding Rd file.

@jorainer
Copy link
Collaborator

jorainer commented Aug 31, 2016

Dear Hendrik, first of all, thanks for your contribution!

General comment (summarizing the above notes):

  • I would put the logic of the second step (adding isotope peaks) into its own function.
  • A detailed documentation of the method is missing: this should contain also motivation, expected results and description of all parameters.
  • I strongly suggest to use individual parameters instead of passing a list with options as a parameter (i.e. the addROIsParams).

… more modular methods; removed parameter list and added individual parameters instead; added documentation for new methods
Merge branch 'devel' of https://github.com/sneumann/xcms into devel

Conflicts:
	NAMESPACE
	R/AllGenerics.R
	R/methods-xcmsRaw.R
	inst/unitTests/runit.findPeaksCentWaveWithIsotopeROIs.R
@sneumann
Copy link
Owner

* checking Rd cross-references ... WARNING
Missing link or links in documentation object 'findPeaks.addPredictedIsotopeFeatures-methods.Rd':
  ‘findPeaks.centWave-methods’

Missing link or links in documentation object 'findPeaks.centWaveWithPredictedIsotopeROIs-methods.Rd':
  ‘findPeaks.addPredictedIsotopeFeatures-methods’
  ‘findPeaks.centWave-methods’

See section 'Cross-references' in the 'Writing R Extensions' manual.

@sneumann
Copy link
Owner

Hi @HendrikTreutler , this still has issues

* checking Rd metadata ... WARNING
Rd files with duplicated alias 'findPeaks.addPredictedIsotopeFeatures':
  ‘findPeaks.addPredictedIsotopeFeatures-methods.Rd’
  ‘findPeaks.centWaveWithPredictedIsotopeROIs-methods.Rd’
* checking Rd cross-references ... OK
* checking for missing documentation entries ... WARNING
Undocumented S4 methods:
  generic 'findPeaks.centWaveWithPredictedIsotopeROIs' and siglist
    'xcmsRaw'
All user-level objects in a package (including S4 classes and methods)
should have documentation entries.

@sneumann sneumann merged commit 39e1823 into sneumann:devel Sep 26, 2016
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.

None yet

3 participants