shabbychef / madness

Multivariate Automatic Differentiation R package
GNU Lesser General Public License v3.0
31 stars 0 forks source link

do not setGeneric. #12

Closed shabbychef closed 7 years ago

shabbychef commented 7 years ago

bug report and patch from Martin Maechler:

Dear Steven,

a recent somewhat innocuous change by me in R-devel (the "next version of R),
namely the addition of a new optional argument 'names' to the diag() function,

is now triggering the following error in the 'madness' examples checks:

-------------------------
> ### Name: as.madness
> ### Title: Coerce to a madness object.
> ### Aliases: as.madness as.madness.default
>
> ### ** Examples
>
> xy <- data.frame(x=rnorm(100),y=runif(100),z=runif(100))
> amod <- lm(z ~ x + y,xy)
> amad <- as.madness(amod)
Error in (function (classes, fdef, mtable)  :
  unable to find an inherited method for function ‘diag’ for signature ‘"numeric"’
Calls: as.madness ... as.madness.default -> madness -> diag -> <Anonymous>
Execution halted
-------------------------

see also
    https://cloud.r-project.org/web/checks/check_results_madness.html )

so the CRAN maintainers (CC) would be happy to receive a new version
of madness eventually (ideally within 10 days or so).

The reason this happens is that you _wrongly_ use setGeneric()
on diag, [[and many other setGeneric(.) on existing (base) functions:]]
That is _not_ a good idea, believe me.
A symptom of that was that you already got the masking warning before:

   > library(madness)
   Attaching package: ‘madness’

   The following object is masked from ‘package:base’:

       diag

and now that has "killed" the functionality.
I append a nice patch file for you, 'madness.diff' which fixes
this case and quite a few others where you should not
setGeneric(<fn>) on an existing function <fn> either but rather
just use setMethod() [and import the <fn> from the relevant
package if that is not base].

(Just use   patch -p0 < madness.diff  when you have
 sub-directory 'madness' in your working directory )

Note that this change also fixes the NAMESPACE (*) and documentation man/*.Rd
files and possibly(*) their roxygen source in R/*.R
  *) possibly: I hope but did not check, as I don't find Roxygen such a good idea
The NAMESPACE and  man/*.Rd files are good in any case.  Note
for the former that it is much better to only importFrom() and
that I explicitly importFrom(Matrix,*) rather there than later
using '::' ... I know that that is partly a matter of taste. I
find the explicit importFrom() more "honest".

About the help file (man/*.Rd; you may think roxygen-stuff)  changes:
The idea is to *not* document (S3 or S4) methods -- unless they have "extra"
arguments, "extra" compared to the generic; we call these
"surprising arguments".

I did successfully run 'R CMD check' on the patched 'madness' package
(but as I said, did _not_ run roxygen2).

Please ask back if you'd like to get more on this.
Maybe I'd prefer to explain in public (R-devel say) why using
setGeneric() in such cases is not a good idea.

Not doing them will your package make more useful notably in the
presence of other packages.

With best regards,
Martin