LamaParama implementation for AdjustRtime(): correcting alignment based on external dataset.

[philouail]

Here I propose the implementation of a new parameter for AdjustRtime() called LamaParama which compute adjustment of retention file by file based on landmarks features (lamas) of an external dataset.

I also implemented some diagnostic function matchLamaChromPeaks(), summarizeLamaMatch to evaluate matching of
chromatographic peaks against lamas before adjustment.

Of note, I also wrote a new interpolation function .force_sorted() which could be reused in other cases and might be moved to another package.

I also wrote an example in the vignette describing everything.

Welcoming any feedback. I'm also not sure that the functions/documentations are in the right files, would appreciate input for that. For example the diagnostic functions should probably have their own documentation ? I'm not completely sure.

[jorainer]

Looks great Phili! I have some general suggestions on restructuring the documentation - having all documentation related to the LaMa method in one place. The code and the vignette looks however great.

[jorainer]

Really great Phili! only very little minor comments. After fixing that please convert to a PR and re-request review, and also review from Steffen.

[jorainer]

Thanks Phili for this great contribution!

I have only one very minor comment in the vignette.

[sneumann]

Hi, I see a new package dependency here. Where is it used ?

I see an explicit library(mgcv) in test_do_adjustRtime-functions.R,
but I suspect it us used somewhere in xcms/R/*.R as well.

I'd love to be able to handle DESCRIPTION and NAMESPACE through roxygen2 one day, and for that we'd need to start annotations
like #' @importFrom MsCoreUtils ndotproduct
Does that make sense for

xcms/R/functions-Params.R

Line 278 in 805ac3f

LamaParama <- function(lamas = matrix(ncol = 2, nrow = 0,

?

Or is the package really only needed in the test_do_adjustRtime-functions.R ?

[philouail]

So in do_adjustRtime-functions.R you will see there is helper function a function called .check_gam_library which will check if mgcv is installed and that is only necessary if the user decide to run a gam model (e.g. method = "gam", default being method = "loess").
The function mgcv:::gam() in this package is then used in the .rt_model() helper function (again only if the user specifically ask for it).
This was a suggestion from @jorainer, if i understood well that allows for the package to only be in suggested and therefore for xcms to not be fully dependent on it, which would not be ideal as only this implementation uses it. Please correct if i'm wrong as I am not 100% confident about that.
I do agree that it would be great if xcms namespace could be regulated by roxygen2, for the other implementations I've done I added these @importFrom mentions (e.g. filterFeatures.R).

Hope that was clear :)

ps: However leaving the library(mgcv) in test_do_adjustedRtime-function.R was a mistake on my part, I forgot to remove it from when I was making the test. I will remove that in the next push !

[jorainer]

Also chiming in here - I absolutely want to avoid any additional (potentially unnecessary) dependency. We can have the mgcv either in Suggests or Enhances since we're not importing any functionality. We're loading the namespace specifically in the function, and only if that method is used. If the package is not installed an error is thrown that, to use the functionality, the mgcv package needs to be installed.

[jorainer]

Yes, maybe it's good (also good practice in general) to already add the @importFrom roxygen directives into the documentation. @philouail , can you please add them to your new functions?

[philouail]

I think it should be good now, just had to move one package around to make it more clear + added the now published force_sorted(), function from MsCoreUtils.

[sneumann]

Excellent, so let's merge now and watch the build reports. Yours, Steffen