[Rd] codetools wrongly complains about lazy evaluation in S4 methods
Martin Maechler
m@ech|er @end|ng |rom @t@t@m@th@ethz@ch
Fri Jun 16 11:41:12 CEST 2023
>>>>> Mikael Jagan
>>>>> on Thu, 15 Jun 2023 22:00:45 -0400 writes:
> On 2023-06-15 5:25 pm, Hervé Pagès wrote:
>> Oh but I see now that you've already tried this in your
>> R/AllGenerics.R, sorry for missing that,
yes, this one:
setGeneric("qr.X",
function(qr, complete = FALSE, ncol, ...)
standardGeneric("qr.X"),
useAsDefault = function(qr, complete = FALSE, ncol, ...) {
if(missing(ncol))
base::qr.X(qr, complete = complete)
else base::qr.X(qr, complete = complete, ncol = ncol)
},
signature = "qr")
where the 'signature = "qr"' is perfect (and as Hervé suggests).
but the complication in useAsDefault = .. is really a bit ugly
... notably if one compares with the nice original base :: qr.X ()
>> but that you worry about the following message being
>> disruptive on CRAN:
>>
>> The following object is masked from 'package:base':
>>
>> qr.X
>>
>> Why would that be? As long as you only define methods for
>> objects that **you** control everything is fine. In other
>> words you're not allowed to define a method for "qr"
>> objects because that method would override
>> base::qr.X(). But the generic itself and the method that
>> you define for your objects don't override anything so
>> should not disrupt anything.
> Yes, maybe it would be fine in principle, and of course
> many popular packages emit startup messages. Still, in
> practice, I think that people are quite accustomed to
> library(Matrix) being "silent", and probably a nontrivial
> fraction of our reverse dependencies would encounter new
> NOTEs about differences between *.Rout and *.Rout.save,
> etc.
I tend to agree with Hervé that the "masked" warning here is
a false positive.
I also agree with Mikael that we would not want this for every
default use of library(Matrix)
I believe we should fix it by using conflictRules(), and from
reading ?conflictRules I understand we could do that by setting
options(conflict.policy = list(mask.ok = "qr.X"))
possibly even in Matrix package's load or attach hook
[[and if really really necessary even as hack inside library()'s checks]]
> The fraction of users who will ever call this method for
> qr.X is very very small compared to the fraction who will
> be confused or annoyed by the message. Hence my hope that
> an implicit generic qr.X could become part of package
> methods, notably as an implicit generic qr.R already lives
> there ...
> Or maybe there is a way for Matrix to define qr.X as an
> implicit generic without creating other problems, but my
> experiments with setGenericImplicit were not promising ...
In principle, I'd say that setGenericImplicit() would be a good
/ "the correct" approach, but as you already tried
unsuccessfully, I'm not claiming the principle.
Martin
> Mikael
>> H.
>> On 6/15/23 13:51, Hervé Pagès wrote:
>>>
>>> I'd argue that at the root of the problem is that your
>>> qr.X() generic dispatches on all its arguments,
>>> including the 'ncol' argument which I think the dispatch
>>> mechanism needs to evaluate **before** dispatch can
>>> actually happen.
>>>
>>> So yes lazy evaluation is a real feature but it does not
>>> play well for arguments of a generic that are involved
>>> in the dispatch.
>>>
>>> If you explicitly defined your generic with:
>>>
>>> setGeneric("qr.X", signature="qr")
>>>
>>> you should be fine.
>>>
>>> More generally speaking, it's a good idea to restrict
>>> the signature of a generic to the arguments "that make
>>> sense". For unary operations this is usually the 1st
>>> argument, for binary operations the first two arguments
>>> etc... Additional arguments that control the operation
>>> like modiflers, toggles, flags, rng seed, and other
>>> parameters, usually have not place in the signature of
>>> the generic.
>>>
>>> H.
>>>
>>> On 6/14/23 20:57, Mikael Jagan wrote:
>>>> Thanks all - yes, I think that Simon's diagnosis ("user
>>>> error") is correct: in this situation one should define
>>>> a reasonable generic function explicitly, with a call
>>>> to setGeneric, and not rely on the call inside of
>>>> setMethod ...
>>>>
>>>> But it is still not clear what the way forward should
>>>> be (for package Matrix, where we would like to export a
>>>> method for 'qr.X'). If we do nothing, then there is
>>>> the note, already mentioned:
>>>>
>>>> * checking R code for possible problems ... NOTE
>>>> qr.X: no visible binding for global variable ‘R’
>>>> Undefined global functions or variables: R
>>>>
>>>> If we add the following to our R/AllGenerics.R :
>>>>
>>>> setGeneric("qr.X", function(qr,
>>>> complete = FALSE, ncol, ...)
>>>> standardGeneric("qr.X"), useAsDefault =
>>>> function(qr, complete = FALSE, ncol, ...) {
>>>> if(missing(ncol))
>>>> base::qr.X(qr, complete =
>>>> complete) else base::qr.X(qr,
>>>> complete = complete, ncol = ncol) },
>>>> signature = "qr")
>>>>
>>>> then we get a startup message, which would be quite
>>>> disruptive on CRAN :
>>>>
>>>> The following object is masked from 'package:base':
>>>>
>>>> qr.X
>>>>
>>>> and if we further add setGenericImplicit("qr.X",
>>>> restore = (TRUE|FALSE)) to our R/zzz.R, then for either
>>>> value of 'restore' we encounter :
>>>>
>>>> ** testing if installed package can be loaded from
>>>> temporary location Error: package or namespace load
>>>> failed for 'Matrix': Function found when exporting
>>>> methods from the namespace 'Matrix' which is not S4
>>>> generic: 'qr.X'
>>>>
>>>> Are there possibilities that I have missed?
>>>>
>>>> It seems to me that the best option might be to define
>>>> an implicit generic 'qr.X' in methods via
>>>> '.initImplicitGenerics' in
>>>> methods/R/makeBasicFunsList.R, where I see that an
>>>> implicit generic 'qr.R' is already defined ... ?
>>>>
>>>> The patch pasted below "solves everything", though we'd
>>>> still have to think about how to work for versions of R
>>>> without the patch ...
>>>>
>>>> Mikael
>>>>
>>>> Index: src/library/methods/R/makeBasicFunsList.R
>>>> ===================================================================
>>>> --- src/library/methods/R/makeBasicFunsList.R
>>>> (revision 84541) +++
>>>> src/library/methods/R/makeBasicFunsList.R (working
>>>> copy) @@ -263,6 +263,17 @@ signature =
>>>> "qr", where = where) setGenericImplicit("qr.R",
>>>> where, FALSE)
>>>>
>>>> + setGeneric("qr.X", + function(qr,
>>>> complete = FALSE, ncol, ...) +
>>>> standardGeneric("qr.X"), + useAsDefault =
>>>> function(qr, complete = FALSE, ncol, ...) {
>>>> + if(missing(ncol))
>>>> + base::qr.X(qr, complete =
>>>> complete) + else base::qr.X(qr,
>>>> complete = complete, ncol = ncol) + },
>>>> + signature = "qr", where = where) +
>>>> setGenericImplicit("qr.X", where, FALSE) + ## our
>>>> toeplitz() only has 'x'; want the generic "here" rather
>>>> than "out there" setGeneric("toeplitz",
>>>> function(x, ...) standardGeneric("toeplitz"),
>>>> useAsDefault= function(x, ...)
>>>> stats::toeplitz(x),
>>>>
>>>> On 2023-06-13 8:01 pm, Simon Urbanek wrote:
>>>>> I agree that this is not an R issue, but rather user
>>>>> error of not defining a proper generic so the check is
>>>>> right. Obviously, defining a generic with
>>>>> implementation-specific ncol default makes no sense at
>>>>> all, it should only be part of the method
>>>>> implementation. If one was to implement the same
>>>>> default behavior in the generic itself (not
>>>>> necessarily a good idea) the default would be ncol =
>>>>> if (complete) nrow(qr.R(qr, TRUE)) else
>>>>> min(dim(qr.R(qr, TRUE))) to not rely on the internals
>>>>> of the implementation.
>>>>>
>>>>> Cheers, Simon
>>>>>
>>>>>
>>>>> On 14/06/2023, at 6:03 AM, Kasper Daniel Hansen
>>>>> <kasperdanielhansen using gmail.com> wrote:
>>>>>>
>>>>> On Sat, Jun 3, 2023 at 11:51 AM Mikael Jagan
>>>>>> <jaganmn2 using gmail.com>
>>>>> wrote:
>>>>>>
>>>>>>> The formals of the newly generic 'qr.X' are
>>>>>>> inherited from the non-generic function in the base
>>>>>>> namespace. Notably, the inherited default value of
>>>>>>> formal argument 'ncol' relies on lazy evaluation:
>>>>>>>
>>>>>>>> formals(qr.X)[["ncol"]]
>>>>>>> if (complete) nrow(R) else min(dim(R))
>>>>>>>
>>>>>>> where 'R' must be defined in the body of any method
>>>>>>> that might evaluate 'ncol'.
>>>>>>>
>>>>>>
>>>>> Perhaps I am misunderstanding something, but I think
>>>>>> Mikael's
>>>>> expectations
>>>>> about the scoping rules of R are wrong. The enclosing
>>>>>> environment
>>>>> of ncol
>>>>> is where it was _defined_ not where it is _called_
>>>>>> (apologies if I am
>>>>> messing up the computer science terminology here).
>>>>>>
>>>>> This suggests to me that codetools is right. But a more
>>>>>> extended
>>>>> example
>>>>> would be useful. Perhaps there is something special with
>>>>>> setOldClass()
>>>>> which I am no aware of.
>>>>>>
>>>>> Also, Bioconductor has 100s of packages with S4 where
>>>>>> codetools
>>>>> works well.
>>>>>>
>>>>> Kasper
>>>>>>
>>>>> [[alternative HTML version deleted]]
>>>>>>
>>>>> ______________________________________________
>>>>> R-devel using r-project.org mailing list
>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>>
>>>>>
>>>>
>>>> ______________________________________________
>>>> R-devel using r-project.org mailing list
>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>> --
>>> Hervé Pagès
>>>
>>> Bioconductor Core Team hpages.on.github using gmail.com
>>
More information about the R-devel
mailing list