ggplot2 dependency #5

faizan-khan-iit · 2017-06-14T15:01:21Z

The motive of this PR is to solve the ggplot2 dependency that animint had in previous versions.

faizan-khan-iit · 2017-06-14T15:12:08Z

@tdhock Ever since I added the ggplot2 code, I see this error message while trying to build the package:

Error in eval(expr, envir, enclos) : could not find function "grob"
Error : unable to load R code in package 'animint2'
ERROR: lazy loading failed for package 'animint2'

I have done the following but the error is recurring:

Using collate I tried to shuffle the order of some relevant files but it didn't work. Currently I use the same collate description as from the ggplot2 package, and then load all the animint code.
I added the Imports and Suggests fields exactly from the ggplot2 code. Still the error persists.

Any ideas??

Edit: Stupid mistake. Forgot to update the NAMESPACE file. Package builds now.
On that note, can we set it up to update automatically??

tdhock · 2017-06-14T15:15:52Z

grob is defined in the grid pacakge, maybe you need to add that to our imports?

tdhock · 2017-06-14T15:16:28Z

more generally you should check the NAMESPACE file from ggplot2 and copy all the stuff that they import

faizan-khan-iit · 2017-06-14T15:31:14Z

@tdhock Could you list the downfalls to this approach?

We fork ggplot2
Rename the package to say ggplot2Animint
Make the changes there and use it for our purposes

I think this way, both of our problems get solved:

We don't have to worry about the ggplot2 version, since users can use the original CRAN version (at least I think its possible)
We still have all the freedom, since we won't be sending the ggplot2 guys any PRs

I was thinking along these lines. Is it possible(to have two versions of ggplot2 with different names in an environment)? If yes, is it doable?

tdhock · 2017-06-14T15:36:34Z

I don't understand, can you please clarify?

faizan-khan-iit · 2017-06-14T15:59:14Z

The problem earlier was that users could only have one version of ggplot2. So our forked version, say v2.1.0, was installed by replacing the CRAN version.

If we change the name to ggplot2Animint (say), the user will still have his CRAN version of ggplot2. So We could depend on our ggplot2Animint fork, so when the user installs animint2, ggplot2Animint gets installed too. That way, we could solve both the problems. The user can have his updated version of ggplot2 from CRAN, and we could use the code in ggplot2Animint for our purposes. If it is possible, that is.

tdhock · 2017-06-14T16:32:01Z

I guess it is possible, but what is the benefit of having the separate ggplot2Animint package? Why not just put everythign in animint2?

faizan-khan-iit · 2017-06-14T16:50:37Z

Yes, I mean it is technically the same thing, but I think it would be neater to just copy their repo along with the testing framework to make sure we don't break anything critical. Additionally we will also need to add our own tests.

Also, like you mentioned, using the new ggplot2Animint package, we could first create static plots before passing them to animint2, much like we used to before. I think it will be much more easily done this way.

Using the current approach, there will always be that task of chipping away the useless code; while I think we could just leave that as it is in the other package, without much hassle. Also, we could easily track any significant additions in the original ggplot2.

And finally just some good programming practice to separate the animation code from the static code. Tracking bugs will be easier if I know that the ggplot2 code is working as it should.

Most of this can be done the current way too. But I would prefer this one. Looks easier and neater.

tdhock · 2017-06-14T16:57:14Z

ok in that case do we change all of geom_ to ageom_ etc? or do we keep geom_?

tdhock · 2017-06-14T16:58:04Z

@cpsievert can you please share your opinion?

faizan-khan-iit · 2017-06-14T17:01:06Z

We will make all the relevant changes in the newer package. For e.g.:

We will change the geom names to geom_a*()
We will not export the functions that are already exported from ggplot2 and are unchanged
Will add the new syntax and tests for allowing the new syntax etc.

tdhock · 2017-06-14T17:06:57Z

for me it seems like making two packages is significantly more complicated than making one package (two times installation, two times CRAN checks, etc) to make sure that ggplot2 code we fork is working as it should, we can just copy their tests to animint2 you know. BTW, their tests are not nearly as thorough as ours. (they don't have a way to test the visual properties of the rendered ggplots, which we do by parsing the generated HTML)

…

On Wed, Jun 14, 2017 at 1:01 PM, Faizan Uddin Fahad Khan < ***@***.***> wrote: We will make all the relevant changes in the newer package. For e.g.: 1. We will change the geom names to geom_a*() 2. We will not export the functions that are already exported from ggplot2 and are unchanged 3. Will add the new syntax and tests for allowing the new syntax etc. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA478qFWo2QgC423RhgbYzljVdnDzuwwks5sEBHSgaJpZM4N6A6p> .

tdhock · 2017-06-14T17:08:55Z

futhermore many of the ggplot2 tests are irrelevant for us. e.g. we won't use the ggplot_build function anymore because it does not know about showSelected, so we don't care whether or not it works.

…

On Wed, Jun 14, 2017 at 1:06 PM, Toby Hocking ***@***.***> wrote: for me it seems like making two packages is significantly more complicated than making one package (two times installation, two times CRAN checks, etc) to make sure that ggplot2 code we fork is working as it should, we can just copy their tests to animint2 you know. BTW, their tests are not nearly as thorough as ours. (they don't have a way to test the visual properties of the rendered ggplots, which we do by parsing the generated HTML) On Wed, Jun 14, 2017 at 1:01 PM, Faizan Uddin Fahad Khan < ***@***.***> wrote: > We will make all the relevant changes in the newer package. For e.g.: > > 1. We will change the geom names to geom_a*() > 2. We will not export the functions that are already exported from > ggplot2 and are unchanged > 3. Will add the new syntax and tests for allowing the new syntax etc. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#5 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/AA478qFWo2QgC423RhgbYzljVdnDzuwwks5sEBHSgaJpZM4N6A6p> > . >

faizan-khan-iit · 2017-06-14T17:33:26Z

for me it seems like making two packages is significantly more complicated than making one package (two times installation, two times CRAN checks, etc)

I was kind of thinking the other way around. The ggplot2 package passes the tests currently and should not have any problems installing, so making changes to allow the new syntax should be easier. Same for animint2.

we won't use the ggplot_build function anymore

Regarding the tests, we could definitely copy them. But from a maintainers perspective, it would be easier with a separate package because:

While we may not use some of the code/tests from ggplot2, it will be time consuming to find it and remove it from the package. Let's leave them as they are in the separate package
Since most of the relevant code will be in animint, we could separate the geom definitions, themes, and facet computations to the ggplot2Animint package that way

Anyway, it is just bookkeeping and I don't think there are any major advantages/disadvantages. Just my personal viewpoint. The only significant difference will be that if we maintain a separate package, we can use that to create static plots for visual testing first, like you mentioned before. I don't think we will be able to do that if we copy the code(?)

faizan-khan-iit · 2017-06-19T15:40:42Z

@tdhock Should we depend on your fork of ggplot2, for consistency?

tdhock · 2017-06-19T15:41:13Z

hey Faizan it seems to be working fine now that I have removed the browser() statements

tdhock · 2017-06-19T15:42:25Z

actually I have to use devtools::load_all() to get it working

tdhock · 2017-06-19T15:42:54Z

> library(animint2)
> g <- ggplot()+geom_point(aes(Sepal.Length, Sepal.Width), data=iris, clickSelects="Species")
> g
> g <- ggplot()+geom_point(aes(Sepal.Length, Sepal.Width), data=iris, clickSelects="Species")
Error in plot_clone(p) : attempt to apply non-function
> devtools::load_all("~/R/animint2")
Loading animint2
> g <- ggplot()+geom_point(aes(Sepal.Length, Sepal.Width), data=iris, clickSelects="Species")
> g
> g <- ggplot()+geom_point(aes(Sepal.Length, Sepal.Width), data=iris, clickSelects="Species")
> g

faizan-khan-iit · 2017-06-19T15:47:43Z

That is weird. Could you please try this? I am still getting the same error:

mtcars$cyl <- as.factor(mtcars$cyl)

p1 <- ggplot() + geom_point(aes(x=hp,
                                y=disp),
                             showSelected = c("cyl", "am"),
                             clickSelects = c("am"),
                             data = mtcars)

p2 <- ggplot() + geom_point(aes(x=hp,
                                y=disp,
                                showSelected = cyl),
                             data = mtcars)

p3 <- ggplot() + geom_point(aes(x=hp,
                                y=disp),
                             data = mtcars)

tdhock · 2017-06-19T15:59:26Z

works for me after load_all:

> library(animint2)
> mtcars$cyl <- as.factor(mtcars$cyl)
> p1 <- ggplot() + geom_point(aes(x=hp,
+                                 y=disp),
+                              showSelected = c("cyl", "am"),
+                              clickSelects = c("am"),
+                              data = mtcars)
Warning: Ignoring unknown parameters: showSelected
> p2 <- ggplot() + geom_point(aes(x=hp,
+                                 y=disp,
+                                 showSelected = cyl),
+                              data = mtcars)
Warning: Ignoring unknown aesthetics: showSelected
> p3 <- ggplot() + geom_point(aes(x=hp,
+                                 y=disp),
+                              data = mtcars)
> p1 <- ggplot() + geom_point(aes(x=hp,
+                                 y=disp),
+                              clickSelects = c("am"),
+                              data = mtcars)
> p3
> p3
Error in plot_clone(plot) : attempt to apply non-function
> devtools::load_all("~/R/animint2")
Loading animint2
> p3
> p1
>

faizan-khan-iit · 2017-06-19T16:09:13Z

Here is what I am doing:

Load All
Build and Reload
Try running the code above

> library(animint2)
> mtcars$cyl <- as.factor(mtcars$cyl)
> p1 <- ggplot() + geom_point(aes(x=hp,
+                                 y=disp),
+                              showSelected = c("cyl", "am"),
+                              clickSelects = c("am"),
+                              data = mtcars)
Warning: Ignoring unknown parameters: showSelected
> p2 <- ggplot() + geom_point(aes(x=hp,
+                                 y=disp,
+                                 showSelected = cyl),
+                              data = mtcars)
Warning: Ignoring unknown aesthetics: showSelected
> p3 <- ggplot() + geom_point(aes(x=hp,
+                                 y=disp),
+                              data = mtcars)
> p1
> p2
Error in plot_clone(plot) : attempt to apply non-function

tdhock · 2017-08-23T11:42:33Z

I fixed the knitr test by changing knit_print.animint2 to knit_print.animint (the S3 method was not being called, so the default print method -- static ggplots -- was being used in the Rmd rendering process)

faizan-khan-iit · 2017-08-24T12:38:31Z

@tdhock We are passing all the tests now except the compiler-gist test. So I guess we could start thinking about closing this PR. Is there anything else that can be done here?

tdhock · 2017-08-24T13:31:46Z

Hey Faizan this is great news, thanks for all your hard work!
I would say let's fix the gist test (finally) before merging this PR.

I just committed the secure string to the .travis.yml file, which I hope will fix the gist issue. otherwise maybe we need to generate a new gist api key and store it in a new travis secure https://docs.travis-ci.com/user/encryption-keys/

faizan-khan-iit · 2017-08-24T15:45:46Z

@tdhock Thanks!

I have added the examples to the package and converted them to the new syntax but I haven't tested them yet. Maybe we could do that in a separate PR?

Also looks like we will have to generate a new gist api key..

tdhock · 2017-08-24T18:17:20Z

hey so I generated a new gist access token under github.com/settings, then I did travis encrypt GITHUB_PAT="ACCES_TOKEN_VALUE" which got me a new secure string that I put in .travis.yml, hopefully this solves the gist issue.

tdhock · 2017-08-24T19:38:34Z

ok, all tests are now passing, and I have updated the version in NEWS/DESCRIPTION/README. I give my official +1 for this PR, go ahead and merge when you are ready.

faizan-khan-iit added 5 commits June 14, 2017 06:23

theme.R -> theme_animint.R

7ae7608

add ggplot2 files

c573957

rename animint files to load last. Use collate

dad5507

use Imports and Suggests from ggplot2 DESCRIPTION

507dae2

add import for packages manually

b72cc9f

import everything like ggplot2

2c73e9a

use local functions instead of exported from ggplot2

7946efc

attempt to apply non function error

589699f

remove browser

9e2d344

faizan-khan-iit and others added 5 commits August 22, 2017 20:00

better params in test-renderer1-variable-value.R

328bcbc

Rookie mistake: forgot to replace with params

f50880b

fix prostateLasso.R test

1918c31

animint -> animint2

499c754

do not import ggplot2, knit_print.animint (not animint2)

c2addf8

faizan-khan-iit added 7 commits August 23, 2017 17:57

Merge branch 'dev' of https://github.com/tdhock/animint2 into dev

d704cca

Remove selector.aes

26d1f4f

Don't need function to add data anymore

8bab5e1

comments, comments, comments

d8d5d1b

documentation

1af81d6

clean up a bit

b4007fa

more comments

bf5ef89

faizan-khan-iit and others added 2 commits August 24, 2017 18:13

add examples to package

b5131f2

try secure for gist

d52f4f9

faizan-khan-iit added 4 commits August 24, 2017 19:10

Merge branch 'dev' of https://github.com/tdhock/animint2 into dev

1662f99

convert examples to new syntax

14f7923

add shiny examples

404c8f5

convert shiny examples

c6c699f

tdhock added 2 commits August 24, 2017 14:16

new GITHUB_PAT

51a5722

Merge branch 'dev' of github.com:tdhock/animint2 into dev

fb13508

version++

b9e9bab

faizan-khan-iit merged commit 55d774b into master Aug 25, 2017

faizan-khan-iit added the enhancement label Aug 28, 2017

faizan-khan-iit mentioned this pull request May 28, 2018

testthat unit test failed faizan-khan-iit/ggplot2#4

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ggplot2 dependency #5

ggplot2 dependency #5

faizan-khan-iit commented Jun 14, 2017

faizan-khan-iit commented Jun 14, 2017 •

edited

Loading

tdhock commented Jun 14, 2017

tdhock commented Jun 14, 2017

faizan-khan-iit commented Jun 14, 2017

tdhock commented Jun 14, 2017

faizan-khan-iit commented Jun 14, 2017

tdhock commented Jun 14, 2017

faizan-khan-iit commented Jun 14, 2017 •

edited

Loading

tdhock commented Jun 14, 2017

tdhock commented Jun 14, 2017

faizan-khan-iit commented Jun 14, 2017

tdhock commented Jun 14, 2017 via email

tdhock commented Jun 14, 2017 via email

faizan-khan-iit commented Jun 14, 2017

faizan-khan-iit commented Jun 19, 2017

tdhock commented Jun 19, 2017

tdhock commented Jun 19, 2017

tdhock commented Jun 19, 2017

faizan-khan-iit commented Jun 19, 2017

tdhock commented Jun 19, 2017

faizan-khan-iit commented Jun 19, 2017

tdhock commented Aug 23, 2017

faizan-khan-iit commented Aug 24, 2017

tdhock commented Aug 24, 2017

faizan-khan-iit commented Aug 24, 2017

tdhock commented Aug 24, 2017

tdhock commented Aug 24, 2017

ggplot2 dependency #5

ggplot2 dependency #5

Conversation

faizan-khan-iit commented Jun 14, 2017

faizan-khan-iit commented Jun 14, 2017 • edited Loading

tdhock commented Jun 14, 2017

tdhock commented Jun 14, 2017

faizan-khan-iit commented Jun 14, 2017

tdhock commented Jun 14, 2017

faizan-khan-iit commented Jun 14, 2017

tdhock commented Jun 14, 2017

faizan-khan-iit commented Jun 14, 2017 • edited Loading

tdhock commented Jun 14, 2017

tdhock commented Jun 14, 2017

faizan-khan-iit commented Jun 14, 2017

tdhock commented Jun 14, 2017 via email

tdhock commented Jun 14, 2017 via email

faizan-khan-iit commented Jun 14, 2017

faizan-khan-iit commented Jun 19, 2017

tdhock commented Jun 19, 2017

tdhock commented Jun 19, 2017

tdhock commented Jun 19, 2017

faizan-khan-iit commented Jun 19, 2017

tdhock commented Jun 19, 2017

faizan-khan-iit commented Jun 19, 2017

tdhock commented Aug 23, 2017

faizan-khan-iit commented Aug 24, 2017

tdhock commented Aug 24, 2017

faizan-khan-iit commented Aug 24, 2017

tdhock commented Aug 24, 2017

tdhock commented Aug 24, 2017

faizan-khan-iit commented Jun 14, 2017 •

edited

Loading

faizan-khan-iit commented Jun 14, 2017 •

edited

Loading