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

methods signature #109

Closed
lgatto opened this issue Jun 30, 2016 · 5 comments
Closed

methods signature #109

lgatto opened this issue Jun 30, 2016 · 5 comments

Comments

@lgatto
Copy link
Owner

lgatto commented Jun 30, 2016

As an example, the peaksCount generic is defined in mzR as

setGeneric("peaksCount", function(object,scans,...) standardGeneric("peaksCount"))

I would suggest to only implement methods for object = "OnDiskMSnExp", scans = "missing" (and ignore scans = "numeric"). If only some peaks count needs to be computer, it seems more consistent to subset prior to calling the methods, like

peaksCount(x[scans])
@lgatto
Copy link
Owner Author

lgatto commented Jun 30, 2016

Done in 08385ed.

@lgatto lgatto closed this as completed Jun 30, 2016
@lgatto
Copy link
Owner Author

lgatto commented Jun 30, 2016

NB: I will keep the 2 in-memory methods for now, as the one with numeric scans should be deprecated prior to defunct. Will do that later.

@lgatto lgatto reopened this Jun 30, 2016
@lgatto
Copy link
Owner Author

lgatto commented Jun 30, 2016

And by the way, the performance is hardly different:

> f <- proteomics(full = TRUE)[1]
> suppressMessages(xx <- MSnbase:::readMSData2(f))
> microbenchmark(suppressMessages(spectra(xx[1:2])), 
+                suppressMessages(spectra(xx, scans = 1:2)))
Unit: milliseconds
                                       expr      min       lq     mean   median
         suppressMessages(spectra(xx[1:2])) 187.1789 191.2811 200.8911 197.4171
 suppressMessages(spectra(xx, scans = 1:2)) 183.9336 186.9881 198.0007 192.6416
       uq      max neval
 203.5280 250.7263   100
 199.1815 256.3811   100

@lgatto lgatto changed the title peaksCount signature methods signature Jun 30, 2016
@jorainer
Copy link
Collaborator

jorainer commented Jul 1, 2016

I just added some additional functionality to peaksCount that prevents the full data to be imported and peak counts being calculated if the data was not processed (returning the originalPeaksCount in that case).
Pushed in 617c477

@lgatto
Copy link
Owner Author

lgatto commented Jul 1, 2016

Ok, thanks.

Regarding you comment here

## @lgatto: I suppose we will do the subsetting all on the object itself, thus
## we don't need the subsetting code below, right?
## Sub-set the feature data based on the arguments.
+    fd <- .subsetFeatureDataBy(fData(object), index=index, scanIdx=scanIdx,
+                               name=name, rtlim=rtlim)

Indeed, I think we can remove this. Will do after pulling/merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants