search

schymane / ReSOLUTION

SOLUTIONS for High ReSOLUTION Mass Spectrometry
7 stars 5 forks source link

RMB_EIC_prescreen: Complain when RMB settings not loaded. #6

Open MaliRemorker opened 5 years ago

MaliRemorker commented 5 years ago

Right now, the thrown error sounds misleading:

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

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 commented 5 years ago

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

schymane commented 5 years ago

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

schymane commented 5 years ago

https://github.com/MassBank/RMassBank/blob/master/R/settings_example.R#L1

meowcat commented 5 years ago

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.