Hervé Pagès
2023-Jun-15 21:25 UTC
[Rd] codetools wrongly complains about lazy evaluation in S4 methods
Oh but I see now that you've already tried this in your R/AllGenerics.R, sorry for missing that, 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. 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 at gmail.com> wrote: >>>> >>>> On Sat, Jun 3, 2023 at 11:51?AM Mikael Jagan <jaganmn2 at 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 at r-project.org mailing list >>>> https://stat.ethz.ch/mailman/listinfo/r-devel >>>> >>> >> >> ______________________________________________ >> R-devel at r-project.org mailing list >> https://stat.ethz.ch/mailman/listinfo/r-devel > -- > Herv? Pag?s > > Bioconductor Core Team > hpages.on.github at gmail.com-- Herv? Pag?s Bioconductor Core Team hpages.on.github at gmail.com [[alternative HTML version deleted]]
Mikael Jagan
2023-Jun-16 02:00 UTC
[Rd] codetools wrongly complains about lazy evaluation in S4 methods
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, 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. 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 ... 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 at gmail.com> wrote: >>>>> >>>>> On Sat, Jun 3, 2023 at 11:51?AM Mikael Jagan <jaganmn2 at 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 at r-project.org mailing list >>>>> https://stat.ethz.ch/mailman/listinfo/r-devel >>>>> >>>> >>> >>> ______________________________________________ >>> R-devel at r-project.org mailing list >>> https://stat.ethz.ch/mailman/listinfo/r-devel >> -- >> Herv? Pag?s >> >> Bioconductor Core Team >> hpages.on.github at gmail.com >