[Bioc-devel] Latest R-devel revision breaks flowCore and other flow packages

Hahne, Florian florian.hahne at novartis.com
Mon Jan 26 09:38:16 CET 2015


Hi Michael,
I'll take a look. In order to improve my code: what exactly do you think
should be part of the initialiser, and what should be in the constructor?
There don't seem to bee any clear guidelines out there anywhere. And if
all logic goes in the constructor, how does one deal with more complex
nested inheritance? And what's the use for the initialiser in the first
place?
Florian


On 24/01/15 03:03, "Michael Lawrence" <lawrence.michael 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.
>
>
>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
>>>
>>>
>>>
>>>
>
>_______________________________________________
>Bioc-devel at r-project.org mailing list
>https://stat.ethz.ch/mailman/listinfo/bioc-devel



More information about the Bioc-devel mailing list