japal / maldirppa Goto Github PK

You use the package testthat in your DESCRIPTION file but doesn't create any unit test.
In my opinion it is very important for a scientific package to provide unit tests to ensure that all functions do what they are supposed to do.

Instead of calculating quantile, mean, median etc. twice to get each limit use temporary variables or vectorisation:

lim <- mean(est, na.rm=TRUE) + sd(est, na.rm=TRUE) + c(threshold, -threshold)

Why you copy&paste mvoutlier::arw and not just depend on the package and use the function from it?

(using your own version of code forces you to look for bugs and updates yourself)

Hello,

Could you please help to solve this issue?

out <- detectOutliers(peaks, by = type$MosID)
Error in cmdscale(x.dist, k = floor(nrow(x)/2) - (i - 1)) :
'k' must be in {1, 2, .. n - 1}

Not sure if that's because my spectra is over the scale limit? Overall, I have 522 spectra but got left after screenSpectra about 483 spectra. I have no issues about this error with less number of spectra analysis.

I'm new here and appreciate with your help,

Thank you,

Best regards,
Rutaiwan

As I already described in #4.

Here are some examples that could be improved by pre-allocating memory:

Already addressed by #4 :

Is there a specific reason why you don't use your MSlist from MSlist.R in importSpectra but rewrite/copy&paste the code into the function?

Just a minor point:

> summary(h)
(10 first mass spectra) 
          ID   A score   Class
1  160408F21 0.1683850 success
2  160408F22 0.1716699 success
3  160408F23 0.1887859 success
4  160408F24 0.1478541 success
5  160408G01 0.1479799 success
6  160408G02 0.1734508 success
7  160408G03 0.1772491 success
8  160408G04 0.1534341 success
9  160408G05 0.1716697 success
10 160408G06 0.1538116 success

----------------------------

Scale estimator: Q 
Method: adj.boxplot 
Threshold: 1.5 
Limits: [0.1196,0.2942] 
Deriv. order: 1 
Lambda: 0.5 
No. potentially faulty spectra: 19 (4.75 %)> plot(h)

expected output:

# ...
No. potentially faulty spectra: 19 (4.75 %)
> plot(h)

So there is just a \n needed.

if (x==TRUE) is superfluous. if expects a condition to be TRUE or FALSE, that means if(x) is enough if x is logical (or numeric). Your current approach is evaluated twice. First x is compared to TRUE and secondly the if tests whether this result is TRUE.

Instead of if (x==FALSE) just use if (!x).

Use if (binary) here.

Use if (!binary) here.

Other lines with the same approach:

As mentioned in #4 MALDIquant offers the [ for MassSpectrum and MassPeaks objects. So the following code in redResolution could be simplified:

if (is.list(x)) {
   x <- lapply(x, function(xx)xx[seq(1, length(xx), by=by)])
} else
   x <- x[seq(1, length(xx), by=by)]
}

&& is different from &, see ?"&&":

‘&’ and ‘&&’ indicate logical AND and ‘|’ and ‘||’ indicate
logical OR. The shorter form performs elementwise comparisons in
much the same way as arithmetic operators. The longer form
evaluates left to right examining only the first element of each
vector. Evaluation proceeds only until the result is determined.
The longer form is appropriate for programming control-flow and
typically preferred in ‘if’ clauses.

Lines:

Dear Javier,
reading your excellent vignette, I tried to follow your standard script and ran into an error:

library(MALDIrppa)
data(spectra)
data(type)
summarySpectra(spectra[1:10])
sc.results <- screenSpectra(spectra, meta = type)

Error in mcComp(x, doReflect, eps1 = eps1, eps2 = eps2, maxit = maxit, :
object 'mc_C' not found

traceback()

5: mcComp(x, doReflect, eps1 = eps1, eps2 = eps2, maxit = maxit, 
       trace.lev = trace.lev)
4: mc.default(x, ..., na.rm = TRUE)
3: mc(x, ..., na.rm = TRUE)
2: adjboxStats(est, coef = threshold)
1: screenSpectra(spectra, meta = type)

sessionInfo()

R version 3.4.3 (2017-11-30)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Debian GNU/Linux 8 (jessie)

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] MALDIrppa_1.0.1   lattice_0.20-35   wmtsa_2.0-3       robustbase_0.91-1 signal_0.7-6      MALDIquant_1.17

Do you see any problems at first sight? Am I using the wrong combination of packages?
Do the base packages interfere with dependencies of MALDIrppa?
This meassage from library(MALDIrppa) may be a hint...:

Attaching package: ‘signal’
The following objects are masked from ‘package:stats’:
    filter, poly

Attaching package: ‘robustbase’
The following object is masked from ‘package:stats’:
    sigma

Thanks for your support and with Best Wishes, Holger

IHMO MSlist and MSlist2 are bad function names. Just from reading the names I had no idea what the functions are doing. Even after reading the man page it is hard to remember that MSlist creates MassSpectrum and MSlist2 MassPeaks objects. Currently I have no good idea for a better name but maybe something like matrixToSpectra/matrixToPeaks ...

Dear Javier,

please don't use nested function definitions. While it is valid R code it is generally considered as bad practice. Because the nested (private) functions life in the namespace/frame of the parent functions they are hard to debug, hard to test (unit tests) and could not be reused by other functions.

An example (functions nested in 2 levels):

Instead of hiding the functions inside another function you could hide your private functions by not exporting them in the NAMESPACE file.

summarySpectra and summaryPeaks are nearly identical. I would suggest to create an internal function that calculates the summary statistics for mass and intensity. This could be used in summarySpectra and summaryPeaks. The snr statistics could be easily cbinded to the out data.frame.

Do you know MALDIquantForeign::importCsv? Do you want to avoid to depend on MALDIquantForeign or are there any other reasons why you do not can/want to use MALDIquantForeign::importCsv?

There are still some pieces of code that could use isMassSpectrumList:

Obviously I don't catch all with #1 .

While I totally understand why you create tranfIntensity I don't understand why you use

spectra <- transfIntensity(spectra, fun = function(x) sqrt(x))

instead of

spectra <- transformIntensity(spectra, "sqrt")

in the vignette?

In earlier versions of MALDIquant there was a fun argument in transformIntensity that accepted user-defined functions. We removed it because many users of MALDIquant don't understand the concept of anonymous functions like function(x)sqrt(x) (BTW: transfIntensity(spectra, fun=sqrt) should be sufficient here). Maybe it was not the best decision we made.

It is good practice to use the accessor methods instead of the slots of a class. See for example: https://stackoverflow.com/questions/9900134/is-it-bad-practice-to-access-s4-objects-slots-directly-using and especially Martin Morgan's answer: https://stackoverflow.com/a/9900822

Beside being good practice it avoids errors (because the accessors add some additional checks), e.g.:

library("MALDIquant")

s <- createMassSpectrum(mass=1:5, intensity=1:5)

## won't work, checks for equal length
intensity(s) <- 1:10
# Error in `intensity<-`(`*tmp*`, value = 1:10) :
#  Lengths of intensity(5) and value (10) have to be equal.

## works, no additional checks
s@intensity <- 1:10

## but now the object is broken
validObject(s)
# Error in validObject(s) :
#  invalid class “MassSpectrum” object: Lengths of mass (5) and intensity (10) have to be equal.

So use mass(x), intensity(x), snr(x) and metaData(x) instead of x@mass, x@intensity, x@metaData, and x@snr wherever possible (the would be some edge cases where you have to access the slots directly).