[Bioc-devel] Latest R-devel revision breaks flowCore and other flow packages
Kasper Daniel Hansen
kasperdanielhansen at gmail.com
Mon Jan 26 15:54:35 CET 2015
I might add that the old (very old) style in Biobase was to use "new" as
constructor. That one should avoid this all the time, is a newer insight,
so you'll find plenty of old code violating this.
Kasper
On Mon, Jan 26, 2015 at 8:35 AM, Hahne, Florian <florian.hahne at novartis.com>
wrote:
> Thanks,
> not much need for convincing here, just wanted a big enough push to get me
> over that 'kinda works so why bother` hurdle.
> I need to do other code refactoring in Gviz soon, and will take the
> opportunity to kick out all initialisers in the process. So one less
> package to worry about.
> Florian
>
> On 26/01/15 14:28, "Michael Lawrence" <lawrence.michael at gene.com> wrote:
>
> >In the S4Vectors/IRanges infrastructure, we have never defined an
> >initialize method, so it is certainly possible to use the constructor
> >pattern in complex frameworks, and IRanges serves as a good example of
> >doing so. The basic pattern is to delegate to the superclass
> >constructor and pass that object as an unnamed argument to new().
> >
> >We stayed away from changing the formals of initialize, because (a)
> >initialize() has a special contract of no-arg invocation that is a
> >pain to support and may not be consistent with the user-facing
> >interface and (b) more importantly, we wanted to preserve the ability
> >(at the low level) to re-initialize based purely on slots. Once you
> >have a custom initialize, it is no longer possible to call new()
> >consistently (just with slots) across all classes. And (c), since in
> >general we want constructors anyway (the user calling new() directly
> >would sacrifice abstraction), this policy greatly reduced complexity
> >by keeping the logic at a single layer. Others might have more to add;
> >I stopped thinking after I got to three ;)
> >
> >Hope that helps,
> >Michael
> >
> >
> >
> >
> >
> >On Mon, Jan 26, 2015 at 12:38 AM, Hahne, Florian
> ><florian.hahne at novartis.com> wrote:
> >> 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
> >>
> >> _______________________________________________
> >> Bioc-devel at r-project.org mailing list
> >> https://stat.ethz.ch/mailman/listinfo/bioc-devel
>
> _______________________________________________
> Bioc-devel at r-project.org mailing list
> https://stat.ethz.ch/mailman/listinfo/bioc-devel
>
[[alternative HTML version deleted]]
More information about the Bioc-devel
mailing list