-
Notifications
You must be signed in to change notification settings - Fork 12
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
plt_add()
#246
plt_add()
#246
Conversation
This was exactly what I was thinking of in the digression here: #236 (comment) Great minds etc. ;-) I really like the idea and think that this is a feature that would get a lot of use. I'll be able to take a look at it tomorrow (or whenever you move it out of draft). |
@grantmcdermott I added docs and a test. You might want to look at this now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. I requested some changes that should be relatively minor to action (hopefully).
@@ -536,6 +536,10 @@ tinyplot.default = function( | |||
... | |||
) { | |||
|
|||
# save for plt_add() | |||
options(tinyplot_last_call = match.call(tinyplot, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent with the rest of the package infrastructure, let's rather save this to the existing .tinyplot_env
. This is what we use in R/get_saved_par.R
, for an example:
Lines 104 to 116 in 02d7806
#' @export | |
get_saved_par = function(when = c("before", "after")) { | |
when = match.arg(when) | |
par_env_name = paste0(".saved_par_", when) | |
return(get(par_env_name, envir = get(".tinyplot_env", envir = parent.env(environment())))) | |
} | |
# (non-exported) companion function(s) for setting the original pars | |
set_saved_par = function(when = c("before", "after"), value) { | |
when = match.arg(when) | |
par_env_name = paste0(".saved_par_", when) | |
assign(par_env_name, value, envir = get(".tinyplot_env", envir = parent.env(environment()))) | |
} |
We'll need to assign NULL values to this call object at startup in R/zzz.R
too, e.g.
Line 46 in 02d7806
assign(".saved_par_after", NULL, envir = get(".tinyplot_env", envir = parent.env(environment()))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I was unable to get this to work. I kept getting an error in the tinyplot.density
tests. The message is obscure, having to do with a do.call
and a substitute
. I just can figure this out.
My guess is that this will disappear by itself when we refactor the density code.
In the meantime, I left TODOs in both files with the appropriate save code that we can eventually.
Feel free to take a shot at it if you want, but I have to give up for now.
This is great. Thanks @vincentarelbundock! |
I’ve been trying to think about ways to make it easier to layer plots. We have
add=TRUE
, but it is not very convenient, because we need to repeat all the same arguments in every call.I can’t decide if this is a good or a terrible idea, but here’s a proof of concept for you to comment on:
plt()
, we save the full call somewhere (global option or other).plt_add()
add=TRUE
This proof of concept PR allows things like:
Note: Facets don’t seem to work in this PR.