Dev regression: recursive facet margins (with `add = TRUE`)

[grantmcdermott]

I came across the following problem which seems to be related but it is not yet fixed by your previous commit. If I apply the following plot several times on the same device, the margins keep growing from plot to plot:

library("tinyplot")

tinyplot(bill_len ~ bill_dep, facet = ~ island, data = penguins)
tinyplot_add(type = "p")

tinyplot(bill_len ~ bill_dep, facet = ~ island, data = penguins)
tinyplot_add(type = "p")

tinyplot(bill_len ~ bill_dep, facet = ~ island, data = penguins)
tinyplot_add(type = "p")

Originally posted by @zeileis in #477

GM: I haven't investigated yet, but I think it might be related to #481 Update: See below.

[grantmcdermott]

Update: Using git bisect I've traced the regression to this PR by @vincentarelbundock: #464

Not that it matters, specifically, but the difference is present in the first commit within that PR.

I don't have time to deep dive right now, but would appreciate someone else confirming on your side. Re-using @zeileis's simple example:

pkgload::load_all()

tinyplot(bill_len ~ bill_dep, facet = ~ island, data = penguins)
tinyplot_add(type = "p")

tinyplot(bill_len ~ bill_dep, facet = ~ island, data = penguins)
tinyplot_add(type = "p")

tinyplot(bill_len ~ bill_dep, facet = ~ island, data = penguins)
tinyplot_add(type = "p")

Test by checking out the first commit SHA below, clearing your R console, running the above script, and then moving on to the next commit SHA and repeat.

git checkout cba25cb # bad (final merge commit)
git checkout 33c29c3 # bad (first commit within the PR)
git checkout 518b5d6 # fine (immediately preceding commit)

[grantmcdermott]

(Realisng that my previous post might come across as passive aggressive / accusatory... Definitely not my intention! Tricky regression to spot at the time---we should try to think of a test that could capture this---and I think I'm the one that gave final merge approval. Just wanted to quickly highlight what I've found.)

[grantmcdermott]

Ah, I think I see the problem.

The order of the "# catch for adding to existing facet plot " and # Save current graphical parameters chunks here were accidentally switched. (The latter needs to come before the former.) I'll revert back to the original and test with latest HEAD and see if that fixes things.

tinyplot/R/tinyplot.R

Lines 657 to 674 in 33c29c3

	# catch for adding to existing facet plot
	if (!is.null(facet) && add) {
	recordGraphics(
	par(get_saved_par(when = "after")),
	list = list(),
	env = getNamespace('tinyplot')
	)
	}
	# Save current graphical parameters
	opar = par(no.readonly = TRUE)
	if (restore.par || !is.null(facet)) {
	if (!is.null(file) || !is.null(width) || !is.null(height)) {
	opar$new = FALSE # catch for some interfaces
	}
	on.exit(par(opar), add = TRUE)
	}
	set_saved_par(when = "before", opar)

[zeileis]

Chapeau, nice detective work! I had looked around a bit and wasn't able to find when/where this was introduced 🧐

---

Re: accusatory. I didn't read your post like that.

I mean you could have just said in which commit the problem was introduced without mentioning explicitly who did it. But the info who did it, might be helpful for finding a solution, e.g., when that person recalls why they did it etc.

So I'm ok with such mentions when they are done matter-of-factly as here. But if either you or @vincentarelbundock feel differently and prefer a different approach, I'm happy to go with that as well.

[grantmcdermott]

Ha, all good. Unnecessarily apologetic from my side, despite years on Linux forums. Let's stick with the direct tags!