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

qmap conflict #200

Open
dicook opened this issue Mar 12, 2013 · 9 comments
Open

qmap conflict #200

dicook opened this issue Mar 12, 2013 · 9 comments
Labels
Milestone

Comments

@dicook
Copy link
Member

dicook commented Mar 12, 2013

The example code for qmap:

library(ggmap)
library(cranvas)
qstate <- map_qdata("state")
qmap(qstate, googleMap = TRUE)

creates a conflict, because there is a qmap in both packages.

@chxy
Copy link
Member

chxy commented Mar 12, 2013

That's why we load ggmap before cranvas - to mask the qmap from ggmap.
If we load cranvas first, then have to use cranvas::map(qstate)

@dicook
Copy link
Member Author

dicook commented Mar 12, 2013

Yep, I realize that. It's really a nuisance to have the same function name in ggmap!

On Mar 12, 2013, at 12:18 PM, Xiaoyue Cheng wrote:

That's why we load ggmap before cranvas - to mask the qmap from ggmap.
If we load cranvas first, then have to use cranvas::map(qstate)


Reply to this email directly or view it on GitHub.


Di Cook
[email protected]

@tengfei
Copy link
Member

tengfei commented Mar 12, 2013

How about make cranvas depends and import on ggmap package in DESCRIPTION,
and export your version of qmap() in NAMESPACE, that should fix the
problem, only downside is you have to put ggmap in 'depends' as well as
'import' which looks not that formal.

On Tue, Mar 12, 2013 at 12:21 PM, dicook [email protected] wrote:

Yep, I realize that. It's really a nuisance to have the same function name
in ggmap!

On Mar 12, 2013, at 12:18 PM, Xiaoyue Cheng wrote:

That's why we load ggmap before cranvas - to mask the qmap from ggmap.
If we load cranvas first, then have to use cranvas::map(qstate)


Reply to this email directly or view it on GitHub.


Di Cook
[email protected]


Reply to this email directly or view it on GitHubhttps://github.com//issues/200#issuecomment-14789402
.

Tengfei Yin
MCDB PhD student
1620 Howe Hall, 2274,
Iowa State University
Ames, IA,50011-2274

@yihui
Copy link
Member

yihui commented Mar 12, 2013

How about using ggmap::qmap() instead of loading the package like library(ggmap)?

@tengfei
Copy link
Member

tengfei commented Mar 12, 2013

If I understand the issue correctly, I think the potential problem is that,
users may load ggmap package accidently after they load cranvas sometime(we
cannot prevent this happen), that is going to override cranvas::qmap. To
avoid this happens, we probably have to set ggmap in dependencies(import
itself is not enough), so library(cranvas) with load ggmap automatically
and mask ggmap::qmap(), even thought, users library(ggmap) after that, it
is not going to override the qmap(), so they have to call ggmap::qmap to
force use the function from ggmap when needed. Another add-on solution is
that, if object for qmap() from both packages are different, we can define
S3/S4 method for qmap, to dispatch method automatically, and call the right
function.

On Tue, Mar 12, 2013 at 1:25 PM, Yihui Xie [email protected] wrote:

How about using ggmap::qmap() instead of loading the package like
library(ggmap)?


Reply to this email directly or view it on GitHubhttps://github.com//issues/200#issuecomment-14793804
.

Tengfei Yin
MCDB PhD student
1620 Howe Hall, 2274,
Iowa State University
Ames, IA,50011-2274

@yihui
Copy link
Member

yihui commented Mar 12, 2013

That is probably an edge case. We need to guarantee our qmap() works in the examples. If the user has to load ggmap at the same time, it is his/her responsibility to solve the conflicts by ::.

@dicook
Copy link
Member Author

dicook commented Mar 12, 2013

True, except the code is supposed to help the user, particularly get up to speed with cranvas. If they accidently load the ggmap library second, then the code breaks, and there is no clue in the code that this is the problem. Either a comment should be added to the code sample, letting the user know that there is a reason why the order of the libraries is this way, or that the ggmap::qmap is explicitly given in the code rather than loading the library.

Do we even have ggmap as a dependency for cranvas??

On Mar 12, 2013, at 1:56 PM, Yihui Xie wrote:

That is probably an edge case. We need to guarantee our qmap() works in the examples. If the user has to load ggmap at the same time, it is his/her responsibility to solve the conflicts by ::.


Reply to this email directly or view it on GitHub.


Di Cook
[email protected]

@tengfei
Copy link
Member

tengfei commented Mar 12, 2013

You are right, it depends, if that's just one case used in example, then
your proposed method is totally fine and sufficient.

I have the same problem before, trying to override qplot()/autoplot() from
ggplot2(), and turn it into a S4 method in ggbio. But the case for me is
very different, users almost always load ggplot2 after they load ggbio,
which caused the problem, so I have to use solution above.

On Tue, Mar 12, 2013 at 1:56 PM, Yihui Xie [email protected] wrote:

That is probably an edge case. We need to guarantee our qmap() works in
the examples. If the user has to load ggmap at the same time, it is
his/her responsibility to solve the conflicts by ::.


Reply to this email directly or view it on GitHubhttps://github.com//issues/200#issuecomment-14795761
.

Tengfei Yin
MCDB PhD student
1620 Howe Hall, 2274,
Iowa State University
Ames, IA,50011-2274

@chxy
Copy link
Member

chxy commented Mar 12, 2013

The only function I borrow from ggmap is get_googlemap, so I've changed it
to ggmap::get_googlemap in the code.

ggmap is not a dependency but a suggestion to cranvas.

On Tue, Mar 12, 2013 at 2:03 PM, dicook [email protected] wrote:

True, except the code is supposed to help the user, particularly get up to
speed with cranvas. If they accidently load the ggmap library second, then
the code breaks, and there is no clue in the code that this is the problem.
Either a comment should be added to the code sample, letting the user know
that there is a reason why the order of the libraries is this way, or that
the ggmap::qmap is explicitly given in the code rather than loading the
library.

Do we even have ggmap as a dependency for cranvas??

On Mar 12, 2013, at 1:56 PM, Yihui Xie wrote:

That is probably an edge case. We need to guarantee our qmap() works in
the examples. If the user has to load ggmap at the same time, it is his/her
responsibility to solve the conflicts by ::.


Reply to this email directly or view it on GitHub.


Di Cook
[email protected]


Reply to this email directly or view it on GitHubhttps://github.com//issues/200#issuecomment-14796169
.

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

4 participants