R-Lum / Luminescence

Development of the R package 'Luminescence'
http://r-lum.github.io/Luminescence/
GNU General Public License v3.0
15 stars 7 forks source link

Remove the S3 is() methods as they are never invoked #208

Closed mcol closed 1 week ago

mcol commented 1 week ago

Although 721b85e has added tests for all those is() methods, looking at coverage results it appears that they are never invoked.

All tests pass and I can't think of a reason to keep them around.

RLumSK commented 1 week ago

@mcol: Can you add a NEWS entry for this; just in case people use it in their scripts?

mcol commented 1 week ago

Thanks for the comments, they made me realise that I've been understanding wrongly what was going on.

I had assumed that those methods would be invoked when calling is(RLum_object), in other words, that the S3 system would dispatch is() to the appropriate is.RLum.* method (which, as I mentioned above, it's not the case).

Actually, these methods return a boolean, that's why I could not cover them with my tests. But then we could start using them internally, in places where now we write:

if (!inherits(object, "RLum.Analysis"))

we could start writing

if (!is.RLum.Analysis(object))

If that's reasonable, then I would close this PR.

RLumSK commented 1 week ago

@mcol: OK, then I suggest we keep the is() S3 methods, but we do not replace the inherits() by the is() dispatch because the dispatch is considerably slower and we inherits() internally in computation sensitive parts of the code.

> t <- 1:10
> microbenchmark::microbenchmark(is(t), inherits(t, "numeric"))
Unit: nanoseconds
                   expr  min   lq    mean median   uq   max neval
                  is(t) 1722 1763 1938.89   1804 1886 11029   100
 inherits(t, "numeric")  246  287  345.22    287  328  5002   100
mcol commented 1 week ago

OK, I'll add tests to hit directly those methods and leave the rest as is.