r-lib / evaluate

A version of eval for R that returns more information about what happened
http://evaluate.r-lib.org/
Other
131 stars 35 forks source link

calling par() can trigger a new recorded plot #10

Closed yihui closed 12 years ago

yihui commented 12 years ago

in the following code, we expect two plots but actually get three

library(evaluate) 
z = evaluate(c("plot(1)", "par(mar=rep(0,4))", "plot(2)")) 
str(z, 1) 
## List of 6
##  $ :List of 1
##   ..- attr(*, "class")= chr "source"
##  $ :List of 2
##   ..- attr(*, "version")= chr "2.14.0"
##   ..- attr(*, "class")= chr "recordedplot"
##  $ :List of 1
##   ..- attr(*, "class")= chr "source"
##  $ :List of 2
##   ..- attr(*, "version")= chr "2.14.0"
##   ..- attr(*, "class")= chr "recordedplot"
##  $ :List of 1
##   ..- attr(*, "class")= chr "source"
##  $ :List of 2
##   ..- attr(*, "version")= chr "2.14.0"
##   ..- attr(*, "class")= chr "recordedplot"

the difference between the first plot and the second is there is an additional component .Primitive("par") in the second recorded plot object; I looked into this issue for a while, and feel the recorded plots which contain the par primitive should be discarded, so this line https://github.com/hadley/evaluate/blob/master/R/graphics.r#L28 may be

length(drawing) == 0 || "par" %in% plot_calls(x)  # only an illustration; I know I repeated myself here
yihui commented 12 years ago

no, this is unsafe; e.g. coplot() can produce a recorded plot object that contains par

I don't know how to deal with this case then

yihui commented 12 years ago

I did this in knitr by checking if the only differences in two recorded plots are layout or par, and it is pretty much hackish, but it seems to be working (I have done several tests on it but it still may not be safe); anyway, you may close this issue now.

jeroen commented 12 years ago

Another example:

evaluate("plot(mtcars)")

gives you 121 plots instead of 1.

yihui commented 12 years ago

Yes, that is because plot.new() is called multiple times, and each time evaluate will save a copy of the current plot.

In knitr, I removed the first 120 objects by comparing them sequentially: if the previous plot is a strict subset of the latter, I will remove the previous one; see is_low_change() in https://github.com/yihui/knitr/blob/master/R/plot.R#L140 This is a very dirty trick.

jeroen commented 12 years ago

For OpenCPU I ran into exactly the same issue. I found another workaround that I think works more generally. I use the PDF device and let it create a new file for every 'page'. Then from the existence of these files, you can infer if a plot is a new plot, or on top of an existing plot.

E.g. I start the evaluation with something like

plotdumpdir <- tempdir();
dir.create(plotdumpdir);
pdf(file.path(plotdumpdir,"plotcount%03d.pdf"), onefile=FALSE);

Now every time that before.plot.new or before.grid.newpage are triggered, I check the index of the current plot from these files:

plotcount <- max(as.numeric(substring(grep("plotcount",list.files(plotdumpdir), value=T),10,12)));

Based on the value of plotcount, I decide to either replace a displaylist or append a new one.

yihui commented 12 years ago

Yes that will work well for high-level plots. For me, I may also want to keep low-level plot changes, e.g.

plot(1)
abline(0, 1)

Sometimes I want two plots to be generated in this case.

There must be a way somewhere inside R to tell if a plot object is complete or not, otherwise how do these graphical devices know when to open a new page? (obviously new pages are not triggered only by plot.new or grid.newpage) Paul is probably the best person to ask.

hadley commented 12 years ago

@Yihui would you consider supplying a patch for evaluate (with tests) that would incorporate those changes? Then all packages that make use of evaluate would benefit.

yihui commented 12 years ago

yes I will try

hadley commented 12 years ago

I wonder if the problem is how evaluate uses (or fails to use) the plot hooks - maybe plots should only be captured from the plot hooks, not from every possible event.

hadley commented 12 years ago

I think this is now fixed - I've added a lot more test cases, but I still may have missed some behaviour. If you see any other problems, please let me know.