S4 Generics and NAMESPACE : justified warning ?

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

S4 Generics and NAMESPACE : justified warning ?

Yohan Chalabi
Dear list,

It seems that a package (pkgB) using another package (pkgA) with S4
generics formed by taking existing functions (for example 'plot') must
not import the existing functions ('plot') in its namespace to avoid
the warning "replacing previous import: plot".

Suppose we use the simple 'import' directive in the name space of
pkgB.

pkgA and pkgB files would be

pkgA/NAMESPACE:
    import(graphics)
    import(methods)
    exportClasses("classA")
    exportMethods("plot")

pkgA/R/pkgA.R:
    setClass("classA", contains = "matrix")
    setMethod("plot", "classA", function(x, y, ...) NULL)

pkgB/NAMESPACE:
    import(methods)
    import(garphics)
    import(pkgA)

Loading pkgB would then generate a warning because the generic 'plot'
in pkgA overwrites the 'plot' function imported from graphics name
space :

Loading required package: pkgA
Warning message:
In namespaceImportFrom(self, asNamespace(ns)) :
  replacing previous import: plot

As far as I understood it, one must explicitly import the functions one
needs in pkgB with 'importFrom' to avoid the previous warning. In
our example, one would then take care to not import the 'plot'
function from garphics name space.

This makes the maintenance of packages using S4 packages rather
tedious because one needs to import exactly what one needs rather than
using the simple 'import' directive.

Moreover, the warning "replacing previous import:" is confusing. In this
case we have a generic replacing its existing function like
'setMethod' would do and not a function replacing another function
with the same name.

IMO it is normal that a function in a previous import can be replaced
by its generic. It should not create a warning. But a warning should
be generated when a generic is replaced by another generic.

Does this make sense ?

This could be implemented in 'namespaceImportFrom' with the patch given
bellow.

It would greatly help the maintanance of packages which use S4
packages, because one could use the simple 'import' directive rather
than 'importFrom'.

Looking forward for your comments.

Regards,
Yohan


#################################################################################

Index: src/library/base/R/namespace.R
===================================================================
--- src/library/base/R/namespace.R (revision 49278)
+++ src/library/base/R/namespace.R (working copy)
@@ -797,8 +797,12 @@
  }
     }
     for (n in impnames)
-        if (exists(n, envir = impenv, inherits = FALSE))
-            warning(msg, " ", n)
+        if (exists(n, envir = impenv, inherits = FALSE)) {
+            if (.isMethodsDispatchOn() && methods:::isGeneric(n, ns)) {
+                if (methods:::isGeneric(n, impenv))
+                    warning("replacing previous generic import: ", n)
+            } else warning(msg, " ", n)
+        }
     importIntoEnv(impenv, impnames, ns, impvars)
     if (register) {
         addImports(self, ns,

#################################################################################


--
PhD student
Swiss Federal Institute of Technology
Zurich

www.ethz.ch

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: S4 Generics and NAMESPACE : justified warning ?

Martin Morgan
Yohan Chalabi wrote:

> Dear list,
>
> It seems that a package (pkgB) using another package (pkgA) with S4
> generics formed by taking existing functions (for example 'plot') must
> not import the existing functions ('plot') in its namespace to avoid
> the warning "replacing previous import: plot".
>
> Suppose we use the simple 'import' directive in the name space of
> pkgB.
>
> pkgA and pkgB files would be
>
> pkgA/NAMESPACE:
>     import(graphics)
>     import(methods)
>     exportClasses("classA")
>     exportMethods("plot")
>
> pkgA/R/pkgA.R:
>     setClass("classA", contains = "matrix")
>     setMethod("plot", "classA", function(x, y, ...) NULL)
>
> pkgB/NAMESPACE:
>     import(methods)
>     import(garphics)
>     import(pkgA)
>
> Loading pkgB would then generate a warning because the generic 'plot'
> in pkgA overwrites the 'plot' function imported from graphics name
> space :
>
> Loading required package: pkgA
> Warning message:
> In namespaceImportFrom(self, asNamespace(ns)) :
>   replacing previous import: plot
>
> As far as I understood it, one must explicitly import the functions one
> needs in pkgB with 'importFrom' to avoid the previous warning. In
> our example, one would then take care to not import the 'plot'
> function from garphics name space.
>
> This makes the maintenance of packages using S4 packages rather
> tedious because one needs to import exactly what one needs rather than
> using the simple 'import' directive.
>
> Moreover, the warning "replacing previous import:" is confusing. In this
> case we have a generic replacing its existing function like
> 'setMethod' would do and not a function replacing another function
> with the same name.

Hi Yohan --

Commenting as a user, there's no guarantee that the 'plot' generic
defined in pkgA is derived from graphics::plot via setGeneric; pkgA
could define it's own generic, and one would want to be informed of the
collision.

Maintenance of packages that have used simple 'import' to pull in all
dependencies is tedious, but using 'import' in some ways undermines
benefits of name spaces (restricting the symbol lookup table to reduce
the number of symbols and the possibility of name collisions, and to
more carefully isolate code inside the package name space to changes in
imported packages or induced by the user). So I think a 'better
practice' is to explicitly import just those functions, classes, etc
that are required by the package. Maintenance of such selective imports
is much less tedious, even with complicated package dependencies. There
is an unreleased Bioconductor package to identify specific imports,
available for R-2.9.* at

  svn export
https://hedgehog.fhcrc.org/bioconductor/branches/RELEASE_2_4/madman/Rpacks/codetoolsBioC

or for the development version of R at

  svn export
https://hedgehog.fhcrc.org/bioconductor/trunk/madman/Rpacks/codetoolsBioC

with functions findExternalDeps and writeNamespaceImports. These are
fairly thoroughly tested, but perhaps not fool-proof.

Martin

>
> IMO it is normal that a function in a previous import can be replaced
> by its generic. It should not create a warning. But a warning should
> be generated when a generic is replaced by another generic.
>
> Does this make sense ?
>
> This could be implemented in 'namespaceImportFrom' with the patch given
> bellow.
>
> It would greatly help the maintanance of packages which use S4
> packages, because one could use the simple 'import' directive rather
> than 'importFrom'.
>
> Looking forward for your comments.
>
> Regards,
> Yohan
>
>
> #################################################################################
>
> Index: src/library/base/R/namespace.R
> ===================================================================
> --- src/library/base/R/namespace.R (revision 49278)
> +++ src/library/base/R/namespace.R (working copy)
> @@ -797,8 +797,12 @@
>   }
>      }
>      for (n in impnames)
> -        if (exists(n, envir = impenv, inherits = FALSE))
> -            warning(msg, " ", n)
> +        if (exists(n, envir = impenv, inherits = FALSE)) {
> +            if (.isMethodsDispatchOn() && methods:::isGeneric(n, ns)) {
> +                if (methods:::isGeneric(n, impenv))
> +                    warning("replacing previous generic import: ", n)
> +            } else warning(msg, " ", n)
> +        }
>      importIntoEnv(impenv, impnames, ns, impvars)
>      if (register) {
>          addImports(self, ns,
>
> #################################################################################
>
>

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: S4 Generics and NAMESPACE : justified warning ?

Yohan Chalabi
In reply to this post by Yohan Chalabi
>>>> "MM" == Martin Morgan <[hidden email]>
>>>> on Tue, 18 Aug 2009 06:15:50 -0700

Hi Martin,

Thanks for your response.

   MM> Commenting as a user, there's no guarantee that the 'plot'
   MM> generic
   MM> defined in pkgA is derived from graphics::plot via setGeneric;
   MM> pkgA
   MM> could define it's own generic, and one would want to be informed
   MM> of the
   MM> collision.

This shouldn't be a problem because generics keep track of the function
and package used.

setGeneric(plot)
str(getGeneric(plot))

   MM>
   MM> Maintenance of packages that have used simple 'import' to pull
   MM> in all
   MM> dependencies is tedious, but using 'import' in some ways
   MM> undermines
   MM> benefits of name spaces (restricting the symbol lookup table
   MM> to reduce
   MM> the number of symbols and the possibility of name collisions,
   MM> and to
   MM> more carefully isolate code inside the package name space to
   MM> changes in
   MM> imported packages or induced by the user). So I think a 'better
   MM> practice' is to explicitly import just those functions,
   MM> classes, etc
   MM> that are required by the package. Maintenance of such selective
   MM> imports
   MM> is much less tedious, even with complicated package
   MM> dependencies. There
   MM> is an unreleased Bioconductor package to identify specific
   MM> imports,
   MM> available for R-2.9.* at


I agree with you that 'importFrom' should be the preferred approach. I
have been using it for some time and I have even wrote my own
functions to automatically generate the NAMESPACE.


But the drawback is that it makes the dependency with other packages
version specific and can become tricky when one has several such
dependencies.

To make the maintenance of packages easier, I would like to use
the suboptimal 'import' so that I do not need to care about this
NAMESPACE/package version issue. But I cannot do that because of an,
IMO, unjustified warning.

regards,
Yohan

--
PhD student
Swiss Federal Institute of Technology
Zurich

www.ethz.ch

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: S4 Generics and NAMESPACE : justified warning ?

Martin Maechler
>>>>> "YC" == Yohan Chalabi <[hidden email]>
>>>>>     on Fri, 28 Aug 2009 16:24:29 +0200 writes:

    >>>>> "MM" == Martin Morgan <[hidden email]>
    >>>>> on Tue, 18 Aug 2009 06:15:50 -0700

    YC> Hi Martin,

    YC> Thanks for your response.

    MM> Commenting as a user, there's no guarantee that the
    MM> 'plot' generic defined in pkgA is derived from
    MM> graphics::plot via setGeneric; pkgA could define it's
    MM> own generic, and one would want to be informed of the
    MM> collision.

    YC> This shouldn't be a problem because generics keep track of the function
    YC> and package used.

    YC> setGeneric(plot)
    YC> str(getGeneric(plot))

yes, but you (Yohan) have provided a patch proposal which made
the warning go away in this one case, and
IIUC Martin Morgan argued that the warning was still needed because
of his case above.


     MM> Maintenance of packages that have used simple 'import'
     MM> to pull in all dependencies is tedious, but using
     MM> 'import' in some ways undermines benefits of name
     MM> spaces (restricting the symbol lookup table to reduce
     MM> the number of symbols and the possibility of name
     MM> collisions, and to more carefully isolate code inside
     MM> the package name space to changes in imported packages
     MM> or induced by the user). So I think a 'better practice'
     MM> is to explicitly import just those functions, classes,
     MM> etc that are required by the package. Maintenance of
     MM> such selective imports is much less tedious, even with
     MM> complicated package dependencies. There is an
     MM> unreleased Bioconductor package to identify specific
     MM> imports, available for R-2.9.* at
     MM>   [...............]

    YC> I agree with you that 'importFrom' should be the preferred approach. I
    YC> have been using it for some time and I have even wrote my own
    YC> functions to automatically generate the NAMESPACE.


    YC> But the drawback is that it makes the dependency with other packages
    YC> version specific and can become tricky when one has several such
    YC> dependencies.

Yes.

    YC> To make the maintenance of packages easier, I would like to use
    YC> the suboptimal 'import' so that I do not need to care about this
    YC> NAMESPACE/package version issue. But I cannot do that because of an,
    YC> IMO, unjustified warning.

Yes, I agree, unjustified in your case.
The question is: is there a patch which does differentiate
between your situation and Martin's scenario -- or does your
patch already do that?

Regards,
Martin Maechler, ETH Zurich

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: S4 Generics and NAMESPACE : justified warning ?

Yohan Chalabi
>>>> "MM" == Martin Maechler <[hidden email]>
>>>> on Mon, 31 Aug 2009 14:51:16 +0200

   MM> Yes, I agree, unjustified in your case.
   MM> The question is: is there a patch which does differentiate
   MM> between your situation and Martin's scenario -- or does your
   MM> patch already do that?


Hi Martin,

Thanks for your response.

Here is an attempt to combine both views.

To check if the generic was derived from the function it wants to
overwrite, the following patch checks if the package associated with
the generic is the same as the one where the function was defined.

regards,
Yohan

Index: src/library/base/R/namespace.R
===================================================================
--- src/library/base/R/namespace.R (revision 49543)
+++ src/library/base/R/namespace.R (working copy)
@@ -797,8 +797,19 @@
  }
     }
     for (n in impnames)
-        if (exists(n, envir = impenv, inherits = FALSE))
-            warning(msg, " ", n)
+        if (exists(n, envir = impenv, inherits = FALSE)) {
+            if (.isMethodsDispatchOn() && methods:::isGeneric(n, ns)) {
+                ## warning if generic overwrites a function which
+                ## it was not derived from
+                genNs <- methods:::slot(get(n, envir = ns), "package")
+                genImpenv <- environmentName(environment(get(n, envir = impenv)))
+                genWarn1 <- (!identical(genNs, genImpenv))
+                ## warning if generic overwrites another generic
+                genWarn2 <- methods:::isGeneric(n, impenv)
+                if (genWarn1 || genWarn2)
+                    warning(msg, " ", n)
+            } else warning(msg, " ", n)
+        }
     importIntoEnv(impenv, impnames, ns, impvars)
     if (register) {
         addImports(self, ns,

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: S4 Generics and NAMESPACE : justified warning ?

Martin Maechler
>>>>> "YC" == Yohan Chalabi <[hidden email]>
>>>>>     on Thu, 3 Sep 2009 12:44:13 +0200 writes:

    >>>>> "MM" == Martin Maechler <[hidden email]>
    >>>>> on Mon, 31 Aug 2009 14:51:16 +0200

    MM> Yes, I agree, unjustified in your case.  The question
    MM> is: is there a patch which does differentiate between
    MM> your situation and Martin's scenario -- or does your
    MM> patch already do that?


    YC> Hi Martin,

    YC> Thanks for your response.

    YC> Here is an attempt to combine both views.

    YC> To check if the generic was derived from the function it
    YC> wants to overwrite, the following patch checks if the
    YC> package associated with the generic is the same as the
    YC> one where the function was defined.

    YC> regards, Yohan

    [.....]

I have now committed your proposal (+-),
and also added <R>/tests/Pkgs/ in order to allow checking such
Namespace <-> S4 and other package dependencies.

Martin Maechler, ETH Zurich

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel