Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build Warning regarding phylobase import #39

Closed
joey711 opened this issue Oct 20, 2011 · 7 comments
Closed

Build Warning regarding phylobase import #39

joey711 opened this issue Oct 20, 2011 · 7 comments
Labels

Comments

@joey711
Copy link
Owner

joey711 commented Oct 20, 2011

This is some obscure namespace issue that may take some time to resolve. Would be nice if phylobase fixed this with an update. Unfortunately, it is difficult to fix their code to solve the problem if this stays an official dependency. Code is pretty entrenched with phylobase.

It might be possible to switch to a "depends" rather than "imports" dependency, which is not encouraged by Bioconductor, but may be justified if this warning is going to stall submission.

@joey711
Copy link
Owner Author

joey711 commented Oct 21, 2011

R CMD check phyloseq

  • checking whether package ‘phyloseq’ can be installed ... WARNING
    Found the following significant warnings:
    Warning: replacing previous import ‘plot’ when loading ‘phylobase’
    Warning: replacing previous import ‘summary’ when loading ‘phylobase’
    See ‘/Users/pjm2/github_packages/phyloseq.Rcheck/00install.out’ for details.

@joey711
Copy link
Owner Author

joey711 commented Oct 26, 2011

It appears that phylobase trivially defines plot() and summary() as S4 generics, then extends them. It works fine for their package, and may also for phyloseq, except that there is a warning during package-build that

 Warning: replacing previous import ‘plot’ when loading ‘phylobase’ 
 Warning: replacing previous import ‘summary’ when loading ‘phylobase’ 

It may be that this is fine on its own, but it is colliding with one of the other package dependencies. ggplot2 is a good suspect, as it has no namespace, but might very well fiddle with plot and summary. Well, except it isn't heavy on S4 methods... Hmm. well, in any case, an obvious fix is to modify the way that phylobase is imported, such that everything but the offending generics "summary" and "plot" are imported.

The plan: Modify phyloseq to be explicit in phylobase import, using importFrom, etc.

(0) Update your own plot-method.r code so that it does not depend on the phylobase plot generic, but rather defines its own.

(1) (Static) Parse the phylobase namespace file, and create a list of functions, methods, and classes that are imported.

(2) (Static) Make a dummy roxygen header that lists all of the import tags necessary to fully import phylobase, less the "plot" and "summery" generics.

(3) (Dynamic) Every time you re-compile the documentation with roxygen, the specific-import commands for phylobase should be re-added to the namespace as if it was just a generic import (less the two problem-causing generics).

Robust?
This should be partially robust to changes by phylobase, because you are only depending on things that exist in the exported space of the package now. In the future, you will need to check that new phyloseq functionality does not depend on new phylobase functionality that was not available during Step 1. If so, Step 1 can be re-run, or the new stuff can be added manually, depending on complexity.

@joey711
Copy link
Owner Author

joey711 commented Oct 26, 2011

As a preliminary test, phylobase will be listed as a dependency for testpkg, to see if it causes the same error. If so, the fix can be applied to testpkg, to see if that also fixes the problem...

@joey711
Copy link
Owner Author

joey711 commented Oct 26, 2011

This preliminary test did not cause any warnings. Having phylobase as an imported package does not cause a warning ALONE. I infer that the phyloseq warning is coming from a collision between two different package dependencies defining "summary" and "plot" as objects S4 generics.

Not sure off the top of my head which package it is. Among the imported packages, the new (ver 2.0) package is a prime suspect. However, the proposed solution remains the same.

@joey711
Copy link
Owner Author

joey711 commented Oct 26, 2011

HOLD THE PHONE!

I apologize to all those other CRAN packages I disparaged with my suspicion. The conflict is actually between phylobase and the Bioconductor package "multtest".

Since I only need 2 functions from multtest, and NONE of its plot or summary related methods, I should be able to do a specific import of the 2 things I need from multtest, in the hopes that that alleviates the conflict.

Testing this hypothesis on testpkg is about to commence...

@joey711
Copy link
Owner Author

joey711 commented Oct 26, 2011

This worked. A specific import of just the mt.maxT() and mt.minP() functions resolves the conflict on both testpkg and phyloseq. The phyloseq package now passes R CMD check phyloseq without Warnings.

This issue is closed until further conflicts arrise between depended packages.

@joey711 joey711 closed this as completed Oct 26, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant