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

RMB_EIC_prescreen: Complain when RMB settings not loaded. #6

Open
MaliRemorker opened this issue Jun 21, 2019 · 5 comments

Comments

Projects
None yet
3 participants
@MaliRemorker
Copy link

commented Jun 21, 2019

Right now, the thrown error sounds misleading:

Error in if (!is.na(deprofile.setting)) msPeaks <- deprofile.scan(msPeaks,  :
  argument is of length zero
@schymane

This comment has been minimized.

Copy link
Owner

commented Jun 21, 2019

My suggestion would be to add a test right at the top of the function and then either exit and ask people to load the settings first [then also need to update documentation] or invoke RmbDefaultSettings() and continue with a warning.
The problem with the latter is that people should ideally be doing the pre-screening using the settings they will then use for the RMassBank workflow.
Alternative: settings file is an input. Thoughts?

@MaliRemorker

This comment has been minimized.

Copy link
Author

commented Jun 21, 2019

Absolutely the former. We must not assume default settings are the right settings.

@schymane

This comment has been minimized.

Copy link
Owner

commented Jun 21, 2019

@meowcat do you know offhand how to test whether settings are loaded in RMB?
Can't find it in the documentation

@schymane

This comment has been minimized.

@meowcat

This comment has been minimized.

Copy link

commented Jun 22, 2019

yes, though that function is of course not exported, so either ::: or directly is.null(getOption("RMassBank")).

Without looking at your specific code, I suggest this pattern we use in RMassBank:

findMsMsHR.direct <- function(msRaw, cpdID, mode = "pH", confirmMode = 0, useRtLimit = TRUE, 
			ppmFine = getOption("RMassBank")$findMsMsRawSettings$ppmFine,
			mzCoarse = getOption("RMassBank")$findMsMsRawSettings$mzCoarse,
			fillPrecursorScan = getOption("RMassBank")$findMsMsRawSettings$fillPrecursorScan,
			rtMargin = getOption("RMassBank")$rtMargin,
			deprofile = getOption("RMassBank")$deprofile,
			headerCache = NULL) { do.stuff() }

In this way, it is never opaque that there are parameters being accessed in a specific function call. Doesn't need to be as granular as this, settings = getOption("RMassBank") would be simpler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.