-
Notifications
You must be signed in to change notification settings - Fork 3
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
create ggplot2 compatible wrappers? #5
Comments
Thanks, it does sound like a good idea!
Would you like to submit a pull request, or do you want me to write
the remaining scale_colo[u]r_cmocean wrappers? I would prefer to avoid a
hard dependency on ggplot2 if possible, so I'm going to add a
if(requireNamespace('ggplot2')) check if that's okay with you.
…--
Best regards,
Ivan
|
I would be happy if you could do this! I'm in the last year of my PhD and shouldn't be doing these kinds of fun programming exercises ;-). Adding the dependency check sounds great. |
The functions scale_color_cmocean, scale_colour_cmocean, scale_fill_cmocean are supposed to mimic similarly-named functions from the viridis package.
What do you think about the API we have as of e55f9bb? (there's a package file ready for |
That looks great to me! In viridis they do implement the two col[o]ur functions separately, probably so that roxygen notices them both and creates docs for both? I haven't checked by trying to install the package to see if it does it correctly. #' @rdname scale_viridis
#'
#' @importFrom ggplot2 scale_fill_gradientn scale_color_gradientn discrete_scale
#'
#' @export
scale_color_viridis <- function(..., alpha = 1, begin = 0, end = 1, direction = 1,
discrete = FALSE, option = "D") {
if (discrete) {
discrete_scale("colour", "viridis", viridis_pal(alpha, begin, end, direction, option), ...)
} else {
scale_color_gradientn(colours = viridisLite::viridis(256, alpha, begin, end, direction, option), ...)
}
}
#' @rdname scale_viridis
#' @aliases scale_color_viridis
#' @export
scale_colour_viridis <- scale_color_viridis Also, and this is probably not related to this issue but might be easy to implement, it may be good to do some input checking in the base cmocean fuction, like in viridisLite::viridis: https://github.com/sjmgarnier/viridisLite/blob/b44c8792478c5406256760f734b47ba70bcce904/R/viridis.R#L145 |
Ah never mind, I just saw that you're not using roxygen but writing the docs in a separate Rd file by hand. :) |
looks like you already check the others, so I added a name check :). Wasn't sure about the indentation you use. |
By the way, should the ggplot wrapper make it possible to specify the
version (as in #1) instead of always returning the latest version of
the palette? Seeing as upstream hasn't received any updates for > 3*
the time between 1.2 and 2.0, I don't think that new versions are
likely to appear, but it still sounds prudent to implement for 100%
reproducibility.
…--
Best regards,
Ivan
|
I second being consistent about allowing version specification. There's not a super high chance that the colours might change, but small tweaks are possible (maybe I'll check in with the cmocean creator to get a sense of what future plans might be coming down the pipeline). |
Hmm I guess that it would be important for future updates and reproducible figures... I agree that including it with the proper defaults is the right approach. |
Clark, thanks for your comment! I have added the version argument. Is
there anything you could dislike about the resulting API?
…--
Best regards,
Ivan
|
Last chance to fix any mistakes in the API before I release it to the
unsuspecting world and have to commit to supporting it until the next
breaking release.
The package (built from current master except
8a3b93b because I found that out
later) passes pre-CRAN checks successfully:
https://artifacts.r-hub.io/cmocean_0.3.tar.gz-ca531461204a4fa09c01c8ebd24f4c20
https://artifacts.r-hub.io/cmocean_0.3.tar.gz-cab35ebe86544cb5acd6f4f1d2819eb6
https://artifacts.r-hub.io/cmocean_0.3.tar.gz-125f6aad43584460b3e88a8a53537e74
https://win-builder.r-project.org/lnxRKaLjoTI5
Should I submit the package with the API we currently have to CRAN?
…--
Best regards,
Ivan
|
looks good to me. It might be useful to write some tests at some point to make sure that future updates don't break any of the intended functionality, but for now it's relatively simple so I would postpone that until the next release. 👍 |
Thanks, submitted |
Sorry, was a bit behind checking up on this, but it looks great to me. Thanks! |
The package viridis has some helper functions to allow the colour scale to be used effectively in ggplot2.
It would be great if this package would have some as well!
I just wrote one for the fill scale:
Created on 2020-11-13 by the reprex package (v0.3.0)
This works, but doesn't yet incorporate an alpha parameter yet.
The text was updated successfully, but these errors were encountered: