Fix calculation of theme defaults #2080

has2k1 · 2017-03-20T22:15:07Z

Issue
At plot time, the calculation of theme elements always fell back
to the default theme. As a consequence, the default theme would
leak into complete themes where some of the elements were
unspecified.

Solution
Be more specific when calculating the theme defaults.

Clean Up
theme_void was insufficiently specified. The solution exposed
missing elements that had to be specified.

Fixes #2058
Fixes #2079

has2k1 · 2017-03-20T23:02:37Z

Tests fail due to assignments that relied on the problematic default theme inheritance that this PR got rid of.
The candidates should be (atleast) some of the results of

$ ag 'theme\$.+?<-'

thomasp85 · 2017-04-03T20:06:29Z

I don't think changing inheritance is necessary. All of these issues seems to come from the fact that theme_void() was incompletely specified, which is the root bug...

has2k1 · 2017-04-04T11:09:19Z

I noticed that it was incompletely specified, but I think if you look closer the complete specification that would make the bug go away would imply that the elements themselves have no inheritance hierarchy. That is, even if you have axis.text = element_blank() you would also need axis.text.x = element_blank().

To be clear, these are the options for what could be meant by complete:

Every possible element is specified.
Every possible element is specified or have a parent that is specified.

This patch recognises the second definition, whereas your (@thomasp85) suggestion seems to imply the first. Also, if it is the first then the line that I removed/relocated would always be redundant as there wouldn't be any unspecified elements.

crsh · 2017-05-25T14:31:38Z

Sorry to bother, but are there any news on this pull request?

has2k1 · 2017-05-25T15:28:11Z

@crsh, I'll look into fixing the failing test case. The first time around, I expected to ask for help with a clearer question. I did not get to it.

has2k1 · 2017-06-01T20:12:50Z

Fails on appveyor but not on travis.
@hadley , any reason for this? Do the visual tests only run appveyor? Otherwise, it seems like a legitimate failure.

hadley · 2017-06-02T16:10:09Z

@has2k1 Do you see the failure if you run locally?

has2k1 · 2017-06-02T18:05:24Z

Locally, I get many failures (with/without this PR maybe something to do with fonts) but not the particular one that fails on appveyor.

hadley · 2017-06-02T19:57:43Z

@lionel- do you have any tools for debugging this sort of failure?

lionel- · 2017-06-03T18:34:03Z

Nothing user-oriented yet, we definitely should add some UI for this.

IIRC, I generated a SVG from the faulty plot manually with vdiffr:::write_svg(), then I displayed the SVG contents with cat(), and compared the outputs on Travis and Appveyor.

We need a verbose expect_doppelganger() variant or option.

hadley · 2017-07-03T16:45:01Z

R/theme.r

@@ -380,7 +380,13 @@ theme <- function(line,

 # Combine plot defaults with current theme to get complete theme for a plot
 plot_theme <- function(x) {
-  defaults(x$theme, theme_get())
+  if (is.null(attr(x$theme, "complete"))) {


When does this branch get triggered?

When using the default theme. The cases for the three branches respectively are.

ggplot(...) + geom_point() ggplot(...) + geom_point() + theme_gray() ggplot(...) + geom_point() + theme(...)

Ah, got it. It might be slightly easier to read if you did:

complete <- attr(x$theme, "complete")

hadley · 2017-07-03T16:45:49Z

@karawoo can you please run the visual tests locally?

@has2k1 could you please add a bullet to NEWS? Are there some unit tests we could add to check that we don't fall back to the old (suboptimal) behaviour?

karawoo · 2017-07-03T20:29:04Z

I get the same failing test that AppVeyor complains about. The title for theme-void.svg is being placed in the center rather than at the left, and it looks like the size of the title text and the amount of space between the plot and the title have changed too.

hadley · 2017-07-03T20:35:06Z

@has2k1 can you reproduce those issues locally yourself?

has2k1 · 2017-07-03T21:09:29Z

I cannot reproduce the failing test.

Also, if I approximate the failing test with

df <- data.frame(x = 1:3, y = 1:3, z = c("a", "b", "a"), a = 1)
plot <- ggplot(df, aes(x, y, colour = z)) +
  geom_point() +
  facet_wrap(~ a)

plot + ggtitle("theme_void") + theme_void()

The title is still in the center.

For the speculation (my system is linux), given that on travis (linux) the test passes, and on appveyor (windows) it fails, could be an issue with the svg driver defaults between different systems. @karawoo, I think you can dismiss this as an explanation if the png and svg results of the code above are different.

has2k1 · 2017-07-03T21:43:01Z

For creating a test, I do not know how wise it would be to mess with the default theme (a global variable) in the test suit.

hadley · 2017-07-05T12:36:39Z

@has2k1 it's fine. Just use on.exit() to restore. Alternatively, you could add a second argument to plot_theme that defaults to theme_get() but you could override in the tests (that's probably a better solution)

hadley

It sounds like @karawoo was saying that after this patch, the title is centred, but it should be left aligned. Can you please confirm?

hadley · 2017-07-13T20:29:14Z

tests/testthat/test-theme.r

@@ -163,6 +163,20 @@ test_that("Complete and non-complete themes interact correctly with ggplot objec
  expect_false(attr(p$plot$theme, "complete"))
  expect_equal(p$plot$theme$text$colour, "red")
  expect_equal(p$plot$theme$text$face, "italic")
+
+  # The final calculated plot theme should not blend a complete theme


Could you please make this a separate test?

karawoo · 2017-07-14T16:42:43Z

Yeah, that's what I was seeing with the title.

has2k1 · 2017-07-14T23:46:39Z

I've specified the title, subtiltle and caption text elements of theme_void so that they take on the same values as in the other themes. This should have theme_void maintain a similar overall arrangement. I am not sure if any more theme elements should be specified to get the tests to pass or to make the current appearance the new normal.

hadley · 2017-07-27T21:51:38Z

@has2k1 looks like there are still changes to the tests.

has2k1 · 2017-07-27T23:01:33Z

Since the tests where generated with theme_void inheriting all unspecified elements from theme_grey, eliminating all the changes would require that theme_void be specified by duplicating the elements in the set them_gray_elements - theme_void_elements. I don't think that is a good idea.

I think there are two alternatives;

The cop-out, decide that theme_void is should not be elaborately specified, find a sufficient level of specification and declare that the new normal.
Elaborately specify theme_void then have theme_grey override (replace) elements in theme_void.

The first is what the last commit was aiming for, the 2nd though a good idea it changes the long standing status of theme_grey as the reference (fully specified) theme.

hadley · 2017-07-28T16:25:08Z

If you believe the behaviour is correct, you need to check in updated visual tests.

**Issue** At plot time, the calculation of theme elements always fell back to the default theme. As a consequence, the default theme would leak into complete themes where some of the elements were unspecified. **Solution** Be more specific when calculating the theme defaults. **Clean Up** `theme_void` was insufficiently specified. The solution exposed missing elements that had to be specified. Fixes tidyverse#2058 Fixes tidyverse#2079

When the theme element does not exist, there should be no assigment to any of it's attributes. There was only a single instance of this.

This makes theme_void have a similar overall layout to other themes. And move tests to separate function.

has2k1 · 2017-08-01T10:56:10Z

I need some help, I cannot generate images that can pass visual tests on my system.

lionel- · 2017-08-01T11:11:34Z

What system are you using? vdiffr is currently broken on some Linux systems, I'm investigating.

has2k1 · 2017-08-01T11:16:50Z

Indeed, I am on a linux system.

lionel- · 2017-08-01T11:31:49Z

Which one?

lionel- · 2017-08-01T11:32:41Z

Also could you provide the result of running these two commands please:

gdtools::version_fontconfig()
gdtools::version_freetype()

has2k1 · 2017-08-01T11:34:59Z

Gentoo Linux

> gdtools::version_fontconfig()
[1] ‘2.12.1’

> gdtools::version_freetype()
[1] ‘2.8.0’

hadley · 2017-10-31T15:08:29Z

@has2k1 would you be willing to update this? I can try and merge in the next week or so.

hadley · 2017-11-01T13:48:33Z

Actually, don't worry, I can do it myself

has2k1 mentioned this pull request Jun 19, 2017

Use rel() more consistently in theme elements, take two #2173

Merged

hadley reviewed Jul 3, 2017

View reviewed changes

has2k1 force-pushed the fix-theme-defaults branch from d8c5f0d to d6a5066 Compare July 3, 2017 21:50

has2k1 force-pushed the fix-theme-defaults branch from a509a7d to 981caef Compare July 6, 2017 18:44

hadley reviewed Jul 13, 2017

View reviewed changes

has2k1 force-pushed the fix-theme-defaults branch from 981caef to 6a4472d Compare July 14, 2017 23:20

has2k1 added 3 commits August 1, 2017 05:38

Fix presumptuous theme element assignment

1184aa5

When the theme element does not exist, there should be no assigment to any of it's attributes. There was only a single instance of this.

Add missing parent elements to theme_void

52ca679

has2k1 added 3 commits August 1, 2017 05:38

Add tests for plot_theme

4021d46

Specify title, subtitle and caption texts

f4d3401

This makes theme_void have a similar overall layout to other themes. And move tests to separate function.

Add new images for theme-void tests

01ecde4

has2k1 force-pushed the fix-theme-defaults branch from c454e51 to 01ecde4 Compare August 1, 2017 10:44

hadley closed this in 6770641 Nov 1, 2017

lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix calculation of theme defaults #2080

Fix calculation of theme defaults #2080

has2k1 commented Mar 20, 2017

has2k1 commented Mar 20, 2017

thomasp85 commented Apr 3, 2017

has2k1 commented Apr 4, 2017

crsh commented May 25, 2017

has2k1 commented May 25, 2017

has2k1 commented Jun 1, 2017

hadley commented Jun 2, 2017

has2k1 commented Jun 2, 2017

hadley commented Jun 2, 2017

lionel- commented Jun 3, 2017

hadley Jul 3, 2017

has2k1 Jul 3, 2017

hadley Jul 3, 2017

hadley commented Jul 3, 2017

karawoo commented Jul 3, 2017

hadley commented Jul 3, 2017

has2k1 commented Jul 3, 2017

has2k1 commented Jul 3, 2017

hadley commented Jul 5, 2017

hadley left a comment

hadley Jul 13, 2017

karawoo commented Jul 14, 2017

has2k1 commented Jul 14, 2017

hadley commented Jul 27, 2017

has2k1 commented Jul 27, 2017

hadley commented Jul 28, 2017

has2k1 commented Aug 1, 2017

lionel- commented Aug 1, 2017 •

edited

Loading

has2k1 commented Aug 1, 2017

lionel- commented Aug 1, 2017

lionel- commented Aug 1, 2017

has2k1 commented Aug 1, 2017

hadley commented Oct 31, 2017

hadley commented Nov 1, 2017

Fix calculation of theme defaults #2080

Fix calculation of theme defaults #2080

Conversation

has2k1 commented Mar 20, 2017

has2k1 commented Mar 20, 2017

thomasp85 commented Apr 3, 2017

has2k1 commented Apr 4, 2017

crsh commented May 25, 2017

has2k1 commented May 25, 2017

has2k1 commented Jun 1, 2017

hadley commented Jun 2, 2017

has2k1 commented Jun 2, 2017

hadley commented Jun 2, 2017

lionel- commented Jun 3, 2017

hadley Jul 3, 2017

Choose a reason for hiding this comment

has2k1 Jul 3, 2017

Choose a reason for hiding this comment

hadley Jul 3, 2017

Choose a reason for hiding this comment

hadley commented Jul 3, 2017

karawoo commented Jul 3, 2017

hadley commented Jul 3, 2017

has2k1 commented Jul 3, 2017

has2k1 commented Jul 3, 2017

hadley commented Jul 5, 2017

hadley left a comment

Choose a reason for hiding this comment

hadley Jul 13, 2017

Choose a reason for hiding this comment

karawoo commented Jul 14, 2017

has2k1 commented Jul 14, 2017

hadley commented Jul 27, 2017

has2k1 commented Jul 27, 2017

hadley commented Jul 28, 2017

has2k1 commented Aug 1, 2017

lionel- commented Aug 1, 2017 • edited Loading

has2k1 commented Aug 1, 2017

lionel- commented Aug 1, 2017

lionel- commented Aug 1, 2017

has2k1 commented Aug 1, 2017

hadley commented Oct 31, 2017

hadley commented Nov 1, 2017

lionel- commented Aug 1, 2017 •

edited

Loading