[Bioc-devel] Latest R-devel revision breaks flowCore and other flow packages
Michael Lawrence
lawrence.michael at gene.com
Sat Jan 24 14:46:12 CET 2015
On Fri, Jan 23, 2015 at 6:03 PM, Michael Lawrence <michafla at gene.com> wrote:
> I have found and fixed the affected initialize cases and will begin
> checking in fixes.
>
> Affected packages: RDAVIDWebService, flowCore (as we know), Gviz,
> ChIPseqR, Pviz, VanillaICE, flowFit.
>
> As an aside, in all of these cases, initialize is implementing logic
> that really belongs in a constructor function. Treating new() as a
> high-level constructor should be discouraged in my opinion. Has
> nothing to do with this bug though.
>
> I have also begun looking through cases outside of initialize. There
> are only about 300 cases of callNextMethod() in the codebase. Great
> majority seem OK so far.
>
I looked at 60 or so that occur inside functions with "..." in their
formals. Gviz had a bunch of "questionable" cases, so I have cleaned
those up. I saw some cases where the Gviz authors had to do extra work
to avoid the bug, so the code is simpler now. There was one other case
in qpgraph.
Will check in my changes now. Maintainers of the mentioned packages
should keep an eye on things, because who knows, I could just be
breaking as many things as I am fixing.
>
> On Fri, Jan 23, 2015 at 12:46 PM, Michael Lawrence <michafla at gene.com> wrote:
>> That's right.
>>
>> On Fri, Jan 23, 2015 at 12:22 PM, Mike <wjiang2 at fhcrc.org> wrote:
>>> Sorry, I just want to clarify some more on this.(maybe useful for others as
>>> well)
>>> What you actually mean is callNextMethod() used to drop both ... and the
>>> other arguments explicitly supplied from the parent call (in my case,
>>> parameters argument).
>>> And now after your fix, both gets passed on and that’s why I should
>>> explicitly select the argument for callNextMethod?
>>>
>>> Thanks.
>>>
>>> Mike
>>>
>>> On 01/23/2015 11:30 AM, Michael Lawrence wrote:
>>>
>>> The bug has existed forever. The commit log may be confusing. The
>>> actual bug (or at least a very undesirable behavior) was in
>>> match.call(), not callNextMethod().
>>>
>>> I think it's still true that callNextMethod() is the natural
>>> invocation. When adding arguments to initialize that you do not want
>>> to pass on (and thus set as slots), it's necessary to use explicit
>>> args. There are other cases where callNextMethod() is exactly what you
>>> want. Like the case in rtracklayer that motivated this fix.
>>>
>>>
>>> On Fri, Jan 23, 2015 at 11:23 AM, Mike <wjiang2 at fhcrc.org> wrote:
>>>
>>> Michael,
>>>
>>> Thanks for the confirmation of the issue. I see you were trying to tackle it
>>> in the latest commits r67467:67472 which apparently haven’t fixed the bug
>>> yet (instead it triggers the error of the existing legacy code in other R
>>> packages like flowCore). I can certainly change the code to explicitly pass
>>> on all the arguments to callNextMethod as you and Martin suggested. I just
>>> wonder if the documentation should drop the sentence of Calling with no
>>> arguments is often the natural way to use callNextMethod and change the
>>> example code (at least before the bug is eventually fixed.) so that users
>>> won’t be misusing it.
>>>
>>>
>>>
>>> Mike
>>>
>>> On 01/23/2015 10:55 AM, Martin Morgan wrote:
>>>
>>> On 01/23/2015 10:52 AM, Michael Lawrence wrote:
>>>
>>> First let me apologize for my failure to read emails. There was a
>>> long-standing bug in the methods package where arguments passed as
>>> "..." to a method would be dropped by callNextMethod(). It turns out
>>> that in all known cases this affects calls to initialize(), probably
>>> because there are many initialize() methods, and new() calls
>>> initialize with "...".
>>>
>>> This case is a very typical one, and Martin's fix is the right one,
>>> according to the (unchanged) documentation of callNextMethod().
>>>
>>> It's in general impossible to detect these cases from static analysis,
>>> since we do not know how the user is calling a method. But catching
>>> them in initialize() should be easy, with some false positives. Just
>>> look for callNextMethod().
>>>
>>> Is it OK for me to checkout all of Bioconductor so that I can perform
>>> that analysis, or will that stress the servers too much? I have a
>>> package that embeds GNU Global to index and search
>>> CRAN/Bioconductor-size repositories for these cases.
>>>
>>>
>>> Hi Michael -- there is no problem checking out all
>>> (https://hedgehog.fhcrc.org/bioconductor/trunk/madman/Rpacks presumably) of
>>> Bioc.
>>>
>>> Thanks! Martin
>>>
>>>
>>> Michael
>>>
>>> On Thu, Jan 22, 2015 at 6:15 AM, Martin Morgan <mtmorgan at fredhutch.org>
>>> wrote:
>>>
>>> On 01/22/2015 12:26 AM, Martin Maechler wrote:
>>>
>>>
>>> Mike <wjiang2 at fhcrc.org>
>>> on Tue, 20 Jan 2015 17:16:37 -0800 writes:
>>>
>>>
>>>
>>> > I don't think it has been addressed yet in the later commits of
>>> R-devel.
>>> > And that piece of code in flowCore package was written long time
>>> ago and
>>> > there is nothing wrong with it as far as I can see.
>>>
>>> You are right.
>>>
>>> I thought Michael Lawrence (member of R Core since last summer!)
>>> was also reading Bioc-devel, so wonder why he has not yet
>>> replied --> CC'ing him
>>>
>>> The changes to R-devel also did break the Matrix package in
>>> exactly the same way. You said
>>>
>>> Here is the |initialize|method for |parameterFilter| which causes the
>>> various errors from flow package vignettes.
>>>
>>> |setMethod("initialize",
>>> signature=signature(.Object="parameterFilter"),
>>> definition=function(.Object, parameters,...)
>>> {
>>> if (!missing(parameters))
>>> parameters(.Object) <- parameters
>>> callNextMethod()
>>> })
>>> |
>>>
>>>
>>>
>>> and I also had a _no argument_ call
>>> callNextMethod()
>>> inside an initialize method.
>>>
>>> I'm pretty sure that if you change (the same as I)
>>>
>>> callNextMethod()
>>> to
>>> callNextMethod(.Object, ...)
>>>
>>> you'll have a version of the code that works both in current R 3.1.2
>>> (and older versions) *and* in the R-devel version.
>>>
>>>
>>> I also pinged Michael??
>>>
>>> What's a precise statement of the problem -- no-argument callNextMethod()
>>> inside initialize? Any suggestions on ways to discover these without relying
>>> on package break during build / check / install?
>>>
>>> Martin Morgan
>>>
>>> Michael L and I were not sure how many other packages or R code this
>>> would influence and he was expecting very very few.
>>>
>>> Best regards,
>>>
>>> Martin Maechler, ETH Zurich
>>>
>>>
>>> > On 01/20/2015 05:06 PM, Dan Tenenbaum wrote:
>>> >> I'm not sure if you were implying that this has already been fixed
>>> in R-devel. Note that the devel build machines currently have r67501
>>> installed, which is later than all the commits you cite above, yet the flow
>>> packages are still broken.
>>>
>>> _______________________________________________
>>> Bioc-devel at r-project.org mailing list
>>> https://stat.ethz.ch/mailman/listinfo/bioc-devel
>>>
>>>
>>>
>>> --
>>> Computational Biology / Fred Hutchinson Cancer Research Center
>>> 1100 Fairview Ave. N.
>>> PO Box 19024 Seattle, WA 98109
>>>
>>> Location: Arnold Building M1 B861
>>> Phone: (206) 667-2793
>>>
>>>
>>>
>>>
More information about the Bioc-devel
mailing list