Skip to content

new evaluate does remove identical plot more aggressively #243

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.

Sign up for GitHub

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

Jump to bottom
Open
cderv opened this issue May 13, 2025 · 6 comments · May be fixed by #248
Open

new evaluate does remove identical plot more aggressively #243

cderv opened this issue May 13, 2025 · 6 comments · May be fixed by #248

Comments

@cderv
Copy link
Collaborator

cderv commented May 13, 2025

This was reported in

Previously, with 0.24.0, this would produce two recorded plot

ev <- evaluate::evaluate(function() {
     hist(cars$speed)
     hist(cars$speed, breaks = "Scott")
 })
 lapply(ev, class)
#> [[1]]
#> [1] "source"
#> 
#> [[2]]
#> [1] "recordedplot"
#> 
#> [[3]]
#> [1] "source"
#> 
#> [[4]]
#> [1] "recordedplot"
 packageVersion("evaluate")
#> [1] '0.24.0'

and knitr would be responsible to keep only one or all depending on fig.keep options

> md <- glue::glue("
```{r}
hist(cars$speed)
hist(cars$speed, breaks = 'Scott')
```
", .open = "{{", .close = "}}")
> knitr::knit(text = md) |> cat()
1/1 [unnamed-chunk-1]

``` r
hist(cars$speed)
hist(cars$speed, breaks = 'Scott')
```

![plot of chunk unnamed-chunk-1](figure/unnamed-chunk-1-1.png)

> knitr::opts_chunk$set(fig.keep="all")
> knitr::knit(text = md) |> cat()
1/1 [unnamed-chunk-1]

``` r
hist(cars$speed)
```

![plot of chunk unnamed-chunk-1](figure/unnamed-chunk-1-1.png)

``` r
hist(cars$speed, breaks = 'Scott')
```

![plot of chunk unnamed-chunk-1](figure/unnamed-chunk-1-2.png)

However, with evaluate 1.0.0, there is now a new logic to detect different plots

evaluate::evaluate(function() {
     hist(cars$speed)
     hist(cars$speed, breaks = "Scott")
 })
#> <evaluation>
#> Source code: 
#>   hist(cars$speed)
#> Plot [7]:
#>   <base> C_plot_new()
#>   <base> palette2()
#>   <base> C_plot_window()
#>   <base> C_title()
#>   <base> C_axis()
#>   <base> C_axis()
#>   <base> C_rect()
#> Source code: 
#>   hist(cars$speed, breaks = "Scott")

So fig.keep = 'all' does not have the effect as there is no second plot to keep now.

Is this change expected?

Or could it be something to check when this was rewritten ?

evaluate/R/watchout.R

Lines 62 to 69 in 1c4492b

plot <- recordPlot()
if (!makes_visual_change(plot[[1]])) {
return()
}
if (!looks_different(last_plot[[1]], plot[[1]])) {
return()
}

@cderv
Copy link
Collaborator Author

cderv commented May 13, 2025 •
edited
Loading

I'll try to see if this is deliberate from

I recognize that keeping identical plots may not be something to do often, but it feels not good that we broke fig.keep behavior on that. I am thinking maybe default to current behavior, and adding a way to opt-in previous one could be a solution to keep both

@cderv
Copy link
Collaborator Author

cderv commented Jun 16, 2025 •
edited
Loading

This happens to be tricky, and I am now thinking this could have been a bug in old behavior 🤔

Here are the differences from old behavior v0.24.0 and new one

Previously length of plot_calls() would have been checked

evaluate/R/graphics.r

Lines 30 to 37 in 6cece5f

is_par_change <- function(p1, p2) {
calls1 <- plot_calls(p1)
calls2 <- plot_calls(p2)
n1 <- length(calls1)
n2 <- length(calls2)
if (n2 <= n1) return(FALSE)

and if lower or equal then is would not be considered a par change

for how example, this would have been the values

> plot_calls(p1)
[1] "C_plot_new"    "palette2"      "C_plot_window" "C_title"       "C_axis"        "C_axis"        "C_rect"       
> plot_calls(p2)
[1] "C_plot_new"    "palette2"      "C_plot_window" "C_title"       "C_axis"        "C_axis"        "C_rect" 
> length(plot_calls(p2))
[1] 7
> length(plot_calls(p1))
[1] 7

They are equal and they would have been considered different plot...

evaluate/R/graphics.R

Line 44 in e1ece68

all(last %in% empty_calls)

evaluate/R/graphics.R

Lines 23 to 28 in e1ece68

if (is.empty(plot)) return(NULL)
last_plot <<- plot
plot
}
})

In the new version, this is different.

Now, if the display list is identical, the plot is ignored

evaluate/R/graphics.R

Lines 14 to 17 in cadec2f

looks_different <- function(old_dl, new_dl) {
if (identical(old_dl, new_dl)) {
return(FALSE)
}

and only if the length of the old display list is less than the length of the new plot display list this is considered different plot

evaluate/R/graphics.R

Lines 19 to 22 in cadec2f

# If the new plot has fewer calls, it must be a visual change
if (length(new_dl) < length(old_dl)) {
return(TRUE)
}

In the specific example shared

hist(cars$speed)
hist(cars$speed, breaks = "Scott")

Those are two calls, but they have the same display list, and this gets filtered out now.

So I wonder if this was a bug before, or considered a feature. But previously evaluating the same plot twice, would lead to two recorded plot

evaluate::evaluate(function() {
    plot(cars$speed)
    plot(cars$speed)
}) |> str(max.level = 1)
#> List of 4
#>  $ :List of 1
#>   ..- attr(*, "class")= chr "source"
#>  $ :List of 2
#>   ..- attr(*, "engineVersion")= int 16
#>   ..- attr(*, "pid")= int 64952
#>   ..- attr(*, "Rversion")=Classes 'R_system_version', 'package_version', 'numeric_version'  hidden list of 1
#>   ..- attr(*, "load")= chr(0) 
#>   ..- attr(*, "attach")= chr(0) 
#>   ..- attr(*, "class")= chr "recordedplot"
#>  $ :List of 1
#>   ..- attr(*, "class")= chr "source"
#>  $ :List of 2
#>   ..- attr(*, "engineVersion")= int 16
#>   ..- attr(*, "pid")= int 64952
#>   ..- attr(*, "Rversion")=Classes 'R_system_version', 'package_version', 'numeric_version'  hidden list of 1
#>   ..- attr(*, "load")= chr(0) 
#>   ..- attr(*, "attach")= chr(0) 
#>   ..- attr(*, "class")= chr "recordedplot"

And now it is not at all

evaluate::evaluate(function() {
    plot(cars$speed)
    plot(cars$speed)
}) |> str(max.level = 1)
#> List of 3
#>  $ :List of 1
#>   ..- attr(*, "class")= chr "source"
#>  $ :List of 2
#>   ..- attr(*, "engineVersion")= int 16
#>   ..- attr(*, "pid")= int 67968
#>   ..- attr(*, "Rversion")=Classes 'R_system_version', 'package_version', 'numeric_version'  hidden list of 1
#>   ..- attr(*, "load")= chr(0) 
#>   ..- attr(*, "attach")= chr(0) 
#>   ..- attr(*, "class")= chr "recordedplot"
#>  $ :List of 1
#>   ..- attr(*, "class")= chr "source"
#>  - attr(*, "class")= chr [1:2] "evaluate_evaluation" "list"

We need a way to restore this. Also because for grid graphics it works ok

evaluate::evaluate(function() {
    library(ggplot2)
    ggplot(penguins) + geom_point(aes(bill_len, bill_dep))
    ggplot(penguins) + geom_point(aes(bill_len, bill_dep))
}) |> str(max.level = 1)
#> List of 7
#>  $ :List of 1
#>   ..- attr(*, "class")= chr "source"
#>  $ :List of 1
#>   ..- attr(*, "class")= chr "source"
#>  $ :List of 4
#>   ..- attr(*, "class")= chr [1:3] "rlang_warning" "warning" "condition"
#>  $ :List of 3
#>   ..- attr(*, "engineVersion")= int 16
#>   ..- attr(*, "pid")= int 123828
#>   ..- attr(*, "Rversion")=Classes 'R_system_version', 'package_version', 'numeric_version'  hidden list of 1
#>   ..- attr(*, "load")= chr(0) 
#>   ..- attr(*, "attach")= chr(0) 
#>   ..- attr(*, "class")= chr "recordedplot"
#>  $ :List of 1
#>   ..- attr(*, "class")= chr "source"
#>  $ :List of 4
#>   ..- attr(*, "class")= chr [1:3] "rlang_warning" "warning" "condition"
#>  $ :List of 3
#>   ..- attr(*, "engineVersion")= int 16
#>   ..- attr(*, "pid")= int 123828
#>   ..- attr(*, "Rversion")=Classes 'R_system_version', 'package_version', 'numeric_version'  hidden list of 1
#>   ..- attr(*, "load")= chr(0) 
#>   ..- attr(*, "attach")= chr(0) 
#>   ..- attr(*, "class")= chr "recordedplot"
#>  - attr(*, "class")= chr [1:2] "evaluate_evaluation" "list"

@hadley
Copy link
Member

hadley commented Jun 16, 2025 •
edited
Loading

I think this pretty clearly implies it's a bug:

evaluate::evaluate(function() {
  plot(cars$speed)
  plot(cars$speed)
})

@hadley
Copy link
Member

hadley commented Jun 16, 2025

Hmmmm, but how do we distinguish that case from

evaluate::evaluate(function() {
  plot(cars$speed)
  1 + 1
})

i.e. how can we know if it's a new plot if nothing on the graphics device has changed?

It's a bit different with grid, because the grobs get unique names:

library(ggplot2)
df <- data.frame(x = 1, y = 1)
ev <- evaluate(function() {
  ggplot(df) + geom_point(aes(x, y))
  ggplot(df) + geom_point(aes(x, y))
})
waldo::compare(ev[[2]], ev[[4]])

@cderv cderv linked a pull request Jun 17, 2025 that will close this issue
Draft
@cderv
Copy link
Collaborator Author

cderv commented Jun 17, 2025 •
edited
Loading

Weird behavior. More than two plots do not work the same with old evaluate

> packageVersion("evaluate")
[1] ‘0.24.0’
> evaluate(function() {
+   plot(cars$speed)
+   plot(cars$speed)
+ }) |> classes()
+ evaluate(function() {
+   plot(cars$speed)
+   plot(cars$speed)
+   plot(cars$speed)
+ }) |> classes()
[1] "source"       "recordedplot" "source"       "recordedplot"
[1] "source"       "recordedplot" "source"       "recordedplot" "source"

So deeper bug ?

Now I am thinking: what are the odds of a user needing two identical plots or more one after the other?
But we do get different behavior with base R graphics and grid graphics

@hadley
Copy link
Member

hadley commented Jun 17, 2025

Yeah, it seems most likely that this is a bug in their code. It's a bit weird that grid and base graphics differ, but that's due to fundamental differences in the way those systems operate (mostly that grid gives every plot element a unique name).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Keeping 2 identical plots r-lib/evaluate
2 participants
@hadley @cderv