Skip to content

plt_add()#246

Merged
grantmcdermott merged 5 commits intograntmcdermott:mainfrom
vincentarelbundock:plt_add
Nov 10, 2024
Merged

plt_add()#246
grantmcdermott merged 5 commits intograntmcdermott:mainfrom
vincentarelbundock:plt_add

Conversation

@vincentarelbundock
Copy link
Collaborator

@vincentarelbundock vincentarelbundock commented Nov 8, 2024

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:

  1. Every time we do plt(), we save the full call somewhere (global option or other).
  2. New function plt_add()
    • Takes the last call,
    • Modifies the explicitly-given argument
    • Sets add=TRUE
    • Evaluates the modifed call

This proof of concept PR allows things like:

library(tinyplot)

plt(Sepal.Width ~ Sepal.Length | Species,
    data = iris, 
    type = type_lm())

plt_add(type = "p")

Note: Facets don’t seem to work in this PR.

@grantmcdermott
Copy link
Owner

grantmcdermott commented Nov 8, 2024

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).

@vincentarelbundock vincentarelbundock marked this pull request as ready for review November 8, 2024 13:18
@vincentarelbundock
Copy link
Collaborator Author

vincentarelbundock commented Nov 8, 2024

@grantmcdermott I added docs and a test. You might want to look at this now.

grantmcdermott
grantmcdermott requested changes Nov 8, 2024
View reviewed changes
Copy link
Owner

@grantmcdermott grantmcdermott left a comment •
edited
Loading

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).

R/tinyplot.R
) {

# save for plt_add()
options(tinyplot_last_call = match.call(tinyplot,
Copy link
Owner

@grantmcdermott grantmcdermott Nov 8, 2024

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:

tinyplot/R/get_saved_par.R

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.

tinyplot/R/zzz.R

Line 46 in 02d7806

assign(".saved_par_after", NULL, envir = get(".tinyplot_env", envir = parent.env(environment())))

Copy link
Collaborator Author

@vincentarelbundock vincentarelbundock Nov 9, 2024

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.

@grantmcdermott grantmcdermott mentioned this pull request Nov 8, 2024
Merged
@grantmcdermott
Copy link
Owner

grantmcdermott commented Nov 10, 2024

This is great. Thanks @vincentarelbundock!

@grantmcdermott grantmcdermott merged commit 93bb0a1 into grantmcdermott:main Nov 10, 2024
@grantmcdermott grantmcdermott mentioned this pull request May 4, 2025
Closed
@vincentarelbundock vincentarelbundock deleted the plt_add branch September 9, 2025 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@grantmcdermott grantmcdermott grantmcdermott requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vincentarelbundock @grantmcdermott