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

geiger dependency removed #28

Closed
mwpennell opened this issue Oct 3, 2013 · 4 comments
Closed

geiger dependency removed #28

mwpennell opened this issue Oct 3, 2013 · 4 comments
Assignees

Comments

@mwpennell
Copy link
Collaborator

now just depends on 'ape' plus the three plotting packages: "ggplot2", "grid", "gridExtra". added geiger to test-helper

i have added geospiza.Rd to arbutus just temporarily so examples still work. will find replacement tomorrow.

also, you mentioned that we could use "suggests" instead of "imports" for the plotting pkgs. what is the advantage/disadvantages of this??

@ghost ghost assigned richfitz Oct 3, 2013
@richfitz
Copy link
Member

richfitz commented Oct 3, 2013

I think if you only use things in the examples, or they're fairly optional then they go in Suggests. But I get confused.

If you use "Suggests" then every time that a function is used it needs to be loaded with require(). So if we use the ggplot/grid stuff in only one place then Suggests might be better. If we use them in a couple of places, then keep it in Imports.

@mwpennell
Copy link
Collaborator Author

hmmm....so ggplot2 is used throughout. plot.phy.ss(), contrasts.var.plot(), contrast.nh.plot() and contrast.asr.plot but gridExtra and grid only used in plot.phy.ss (and internal fxns within it).

thinking maybe use require(grid, gridExtra) in plot.phy.ss?? and just suggest them. not sure how many people will want to look at these plots anyways...thoughts?

@richfitz
Copy link
Member

richfitz commented Oct 3, 2013

Sounds like a good plan to me. If it turns out that gridExtra is used elsewhere, we can easily make it required. But not making it required makes it simpler for people to install. ggplot is pretty common these days, and everyone using this package will have ape.

But I don't think it's the end of the world to leave it in Imports either.

@mwpennell
Copy link
Collaborator Author

done. tested the fxns and this works well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants