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

Version 0.2.0 #19

Merged
merged 45 commits into from
Sep 13, 2020
Merged

Version 0.2.0 #19

merged 45 commits into from
Sep 13, 2020

Conversation

evanbiederstedt
Copy link
Collaborator

  • includes refactoring with rasterise() function
  • revised vignettes and README

This was referenced Sep 11, 2020
@evanbiederstedt
Copy link
Collaborator Author

@VPetukhov One thing to check is if the vignettes look acceptable to you.

After this, we can upload to CRAN (which may require some changes) and version afterwards.

@VPetukhov
Copy link
Owner

VPetukhov commented Sep 11, 2020

Hey @evanbiederstedt,
My only concern here is with geom_boxplot_jitter that uses the raster parameter. It's an inheritance of my old mistake, when I didn't create a separate geom_boxplot_jitter_rast function. So I wonder whether we need to fix it now? Btw, should it be "rasteriser" instead of "rasterpoint" in there.

UPDATE: the vignette is cool, with the only small thing about PrintFileSize for boxplots: there are to few dots to understand whether raster would give any advantage. So now looking at the numbers in the vignette I can't tell if rasterization in geom_boxplot_jitter actually works. Why did you choose to give points_num <- 1000000 as an exercise for users? :)

@evanbiederstedt
Copy link
Collaborator Author

Hi there @VPetukhov

My only concern here is with geom_boxplot_jitter that uses the raster parameter. It's an inheritance of my old mistake, when I didn't create a separate geom_boxplot_jitter_rast function. So I wonder whether we need to fix it now?

Yeah, I realize it's using the legacy code. After failing to get jitter working as expected with rasteriser() a few weeks ago, I rationalized to myself this was a good solution. A "good enough" solution.

Btw, should it be "rasteriser" instead of "rasterpoint" in there.

Good catch! Will change. I forget now why I used "rasterpoint"....

Why did you choose to give points_num <- 1000000 as an exercise for users? :)

It's a bit annoying, I agree. The issue is, the vignettes are timed by CRAN, and they need to be under a certain time limit. That's why I came up with this crazy solution.

We could try a larger number---I'll try two magnitudes greater or so

@evanbiederstedt
Copy link
Collaborator Author

evanbiederstedt commented Sep 11, 2020

@VPetukhov

Btw, should it be "rasteriser" instead of "rasterpoint" in there.

RE: "rasterpoint"

So, it cannot be "rasteriser" for here or this break the code. As far as I know, we could use any value we wish for this,
e.g. "fooBar", "smetannik" "flibbertigibbet", "mpochemuchka", it all works :)

Ideas? Or is "rasterpoint" sufficient?

I think that's the last thing to decide, then we are set to merge!

@VPetukhov
Copy link
Owner

As far as I know, we could use any value we wish for this, e.g. "fooBar", "smetannik" "flibbertigibbet", "mpochemuchka", it all works :)

Are you sure? :) If so, I'm fine with both "rasterpoint" or "mpochemuchka". But my first-glance impression was that its goal was to match the makeContext.rasteriser function. Particularly, even in the latest version of the vignette there is no difference in the size between 'Raster' and 'Vector'. Which makes me think that "rasterpoint" doesn't work.

@evanbiederstedt
Copy link
Collaborator Author

evanbiederstedt commented Sep 12, 2020

Are you sure? :) If so, I'm fine with both "rasterpoint" or "mpochemuchka". But my first-glance impression was that its goal was to match the makeContext.rasteriser function. Particularly, even in the latest version of the vignette there is no difference in the size between 'Raster' and 'Vector'. Which makes me think that "rasterpoint" doesn't work.

You're entirely right---I was confused yesterday (i.e. very wrong). I have figured out what had been bothering me, but it's not worth while getting into.

Viktor's original code:

points_num <- 5000000
> PrintFileSize(gg_box_rast, 'Raster')
## Raster: 9822.836 Kb.
> PrintFileSize(gg_box_vec, 'Vector')
## Vector: 9823.248 Kb.

Correcting the mistake here:

class(grob) <- c("rasterpoint", class(grob))

> points_num <- 5000000
> PrintFileSize(gg_box_rast, 'Raster')
Raster: 9805.084 Kb.
> PrintFileSize(gg_box_vec, 'Vector')
Vector: 9806.019 Kb.

Sadly, "mpochemuchka" isn't valid:

> points_num <- 5000000
> PrintFileSize(gg_box_rast, 'Raster')
## Raster: 9792.231 Kb.
> PrintFileSize(gg_box_vec, 'Vector')
## Vector: 9792.215 Kb.

So, I'll change to "rasteriser", and then if acceptable, we can merge/version.

@evanbiederstedt
Copy link
Collaborator Author

evanbiederstedt commented Sep 12, 2020

@VPetukhov I think I figured out the problem; it should be working now.

> points_num <- 5000000
> PrintFileSize(gg_box_rast, 'Raster')
## Raster: 112.9795 Kb.
> PrintFileSize(gg_box_vec, 'Vector')
## Vector: 213.5098 Kb.

I cannot explain to myself why the numbers were so skewed previously for vector plots...

@VPetukhov
Copy link
Owner

Thank you so much, @evanbiederstedt! Now we're finally done with that! :)

@VPetukhov VPetukhov merged commit 1fe1887 into master Sep 13, 2020
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

Successfully merging this pull request may close these issues.

3 participants