Skip to content

Themes#258

Merged
grantmcdermott merged 78 commits intograntmcdermott:mainfrom
vincentarelbundock:tinytheme
Dec 11, 2024
Merged

Themes#258
grantmcdermott merged 78 commits intograntmcdermott:mainfrom
vincentarelbundock:tinytheme

Conversation

@vincentarelbundock
Copy link
Copy Markdown
Collaborator

@vincentarelbundock vincentarelbundock commented Nov 15, 2024

(Includes GM edits)

Closes #234
Closes #112

Out of scope or not PR-specific. Should be a separate issue:

  • Align grid and axis labels #263 Not PR-specific, but more visible in "minimal" theme.
  • Add space between ylab and axis automatically. This is important when axis labels are horizontal, as is default in many themes. Default axis spacing parameters #112 Update: addressed by 9d1380b
  • Plot region is unbalanced when the title and subtitles are both at top. There is large white space at the bottom and too little white space at the top. Update: addressed by 9d1380b

@grantmcdermott

This comment was marked as outdated.

@vincentarelbundock

This comment was marked as outdated.

@vincentarelbundock

This comment was marked as outdated.

@vincentarelbundock

This comment was marked as outdated.

@grantmcdermott

This comment was marked as outdated.

@vincentarelbundock

This comment was marked as outdated.

@grantmcdermott
Copy link
Copy Markdown
Owner

I've just pushed what should hopefully be a simpler (safer?) workaround for setting and reverting themes. The basic idea is that "default" initially retrieves the user's base graphics settings---which are all held in par()---on load. But the "default" theme here is also special, insofar as calling tinytheme()/tinytheme("default") completely resets the "before.plot.new()" hook after instantiating the original pars. Resetting the hook is important because it allows us to start from a clean slate midway through a session and also means that regular (t)par(x = y) calls are recognized as part of the updated user defaults. E.g. You can run:

tinytheme("dark")
plt(1:10)
tinytheme()
par(las = 1)
plt(0:10)

and the second plot will have horizontal axis labels even though they were specified at load time.

I've tested with Quarto and everything seems to be working as expected there too. Feel free to kick the tyres yourself.

Does it work with very long yaxis labels, lie if you multiply the y var by 1000000?

Not yet, but I'll take a crack at that next; hopefully tomorrow.

@grantmcdermott
Copy link
Copy Markdown
Owner

P.S.

Instead of the default tinytheme(NULL) I would find it more intuitive to have an explicit tinytheme("default").

This is super pedantic since most users would just call tinytheme() anyway... but I think the argument for the NULL -> "default" change is weaker with the updated code logic. For one thing, "default" here involves resetting to an original state... but that original state can also change as the user invokes manual par(x=y) calls during the session. Personally, I'm tempted to revert back to tinytheme(NULL) if you two agree.

@zeileis
Copy link
Copy Markdown
Collaborator

zeileis commented Dec 2, 2024

I don't have very strong opinions on this - but I also don't fully get what you are proposing. Do you want to get rid of theme_default() entirely or do you just want to have theme = NULL as the default rather than theme = "default"?

My feeling is still that having an explicit theme_default() and using theme = "default" makes it easier for the user to understand what is going on and to find the corresponding documentation.

But maybe I'm also missing the details of the new logic of the implementation...I haven't been following this thread very carefully. By calling tinytheme(theme = ...) are tpar() settings overwritten or preserved? And par() settings are preserved?

@vincentarelbundock
Copy link
Copy Markdown
Collaborator Author

Tried it on my computer, and things look great. Thanks for the better solution!

Looks like you reverted to boldface for the titles. I feel pretty strongly that all our themes except default should use normal weight for the main title. The title is already at the top, and in larger size. Like Robert Bringhurst says: there's no need to shout at your readers. (Also, bold is just ugly, and we're trying to draw pretty plots.)

I also kind of agree with Achim that "default" is more natural, despite the slight complications in implementation that you note. But I don't feel as strongly about that one.

@grantmcdermott
Copy link
Copy Markdown
Owner

Update: Dynamic margin adjustment is turning out to be a lot harder than expected. Every time I fix one issue, two more appear. Ironically, one of the main sticking points is our new hook mechanism, which conflicts with some of the just-in-time adjustments that we need to make after measuring label widths etc. :-/

I'm still hoping to plug the remaining gaps, but will probably take a few days at least.

@grantmcdermott
Copy link
Copy Markdown
Owner

Okay, another update. I think I've got dynamic margin adjustment mostly working. Still a few rough edges, but quick demo below.

First, a basic plot for comparison:

tinyplot(
  Sepal.Length ~ Petal.Length | Species, data = iris,
  main = "Dynamic plot adjustment and whitespace reduction",
  sub = "For themes with las = 1, etc."
)

Next, a basic "dark" theme example. Key thing I want to draw attention to is that excess whitespace (esp. around the axis labels and titles) has been removed:

tinytheme("dark")
tinyplot(
  Sepal.Length ~ Petal.Length | Species, data = iris,
  main = "Dynamic plot adjustment and whitespace reduction",
  sub = "For themes with las = 1, etc."
)

Next, a fancier version with a legend but—more importantly—long y-axis labels. Note the left-hand plot margin adjusts to make space for this longer text.

tinyplot(
  I(Sepal.Length*1e9) ~ Petal.Length | Species, data = iris,
  main = "Dynamic plot adjustment and whitespace reduction",
  sub = "For themes with las = 1, etc."
)

Two more examples, just demonstrating that dynamic adjustment also carries over to faceted plots.

tinytheme("clean")
tinyplot(
  I(mpg*1e3) ~ hp | disp, data = mtcars, facet = cyl ~ am,
  main = "Dynamic plot adjustment and whitespace reduction",
  sub = "Works with facets too"
)

tinytheme("dark")
tinyplot(
  I(mpg*1e3) ~ hp | disp, data = mtcars, facet = cyl ~ am,
  main = "Dynamic plot adjustment and whitespace reduction",
  sub = "Works with facets too"
)

# reset theme
tinytheme()

@vincentarelbundock
Copy link
Copy Markdown
Collaborator Author

Wow! This looks amazing!!!

Out of curiosity, does the auto margin parameter in tpar() override all user supplied settings, or is it still possible for them to change?

For example, if I think that main and sub are too tight with one a other, can I add some spacing?

@vincentarelbundock
Copy link
Copy Markdown
Collaborator Author

Either way, I think this PR is getting massive, and that we should think about merging soon. Then, we can iterate to improve the themes and add flexibility where needed.

@grantmcdermott
Copy link
Copy Markdown
Owner

grantmcdermott commented Dec 9, 2024

Out of curiosity, does the auto margin parameter in tpar() override all user supplied settings, or is it still possible for them to change?

For example, if I think that main and sub are too tight with one a other, can I add some spacing?

Yes and no. It's a bit complicated, but I'll try to explain briefly:

I ended up deciding to narrowly interpret "dynamic" as limited to las adjustment. (That's not completely true b/c there is some minor legend adjustment stuff that happens too, but just go with it for now.)

To that end, the whitespace reduction specifically around the axis titles etc. is actually baked into the theme default mar and mpg settings (see here). We could easily make these adjustments dynamic too (i.e., only kick in if tpar("dynmar")==TRUE) and, indeed, this is how I had originally implemented it. Upon reflection, however, I decided that it made more sense to make this part of the "hardcoded" theme settings. Thus, the whitespace around the axis titles (for the relevant themes) is reduced irrespective of whether we are also trying to dynamically adjust for horizontal y-axis text too. Basically, I think that whitespace reduction around the axis ticks and titles should be a fixed feature of the theme, rather than relying on some clever internal tricks to adjust spacing dynamically. (Let me know if you disagree.)

Okay, now to your question. Here's a default plot for the "bw" theme:

tinytheme("bw")
tinyplot(
  I(Sepal.Length*1e9) ~ Petal.Length, data = iris,
  main = "Title of the plot",
  sub = "Subtitle of the plot"
)

This theme includes an extra 0.5 line whitespace penalty around the margin of the axis titles (and a similar adjustments for shorter tick lines, but we can ignore that for this example). To undo this whitespace reduction, we could do:

# remove (undo?) extra 0.5 whitespace reduction around axis titles
tinytheme("bw",
          mar = c(5.1, 4.1, 4.1, 2.1) - c(1+0.3, 0.3, 0, 1.5),
          mgp = c(3, 1, 0) - c(0.3, 0.3, 0))
tinyplot(
  I(Sepal.Length*1e9) ~ Petal.Length, data = iris,
  main = "Title of the plot",
  sub = "Subtitle of the plot"
)

(Note the axis titles have more space around them.)

For example, if I think that main and sub are too tight with one a other, can I add some spacing?

This, unfortunately we can't change right now since main and sub do not inherit the same mgp logic as the axes. So we'd either have to connect the latter, or introduce a new tpar parameter that explicitly governs spacing between the title and subtitle.

(Another option would be to include a linebreak in either main or sub, but that too is not something I've enabled checking and adjustment for yet.)

@grantmcdermott
Copy link
Copy Markdown
Owner

Either way, I think this PR is getting massive, and that we should think about merging soon. Then, we can iterate to improve the themes and add flexibility where needed.

Agree! I need to jump to some work stuff now, but will make a list of STILL TODO items this and then we can decide what squeeze in now versus address in a later PR.

@grantmcdermott
Copy link
Copy Markdown
Owner

Fixed a few more issues, added tests, and updated the docs.

I think that this is in pretty good shape now. Still some rough edges here or there (e.g., themes don't play nicely with "spineplot" or "ridge"). But absent any objections, tomorrow I plan to update the vignette(s) and then merge.

@vincentarelbundock
Copy link
Copy Markdown
Collaborator Author

absent any objections, tomorrow I plan to update the vignette(s) and then merge.

Yay!

I think the next release is going to be a game changer

@zeileis
Copy link
Copy Markdown
Collaborator

zeileis commented Dec 10, 2024

+1 The updates are massive and it's very impressive what you have put together with the types and themes.

@grantmcdermott grantmcdermott merged commit e75358b into grantmcdermott:main Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tinyplot themes Default axis spacing parameters

3 participants