Skip to content

Add tag label for adding identifier to plot#2405

Merged
hadley merged 13 commits into
tidyverse:masterfrom
thomasp85:label-tag
May 9, 2018
Merged

Add tag label for adding identifier to plot#2405
hadley merged 13 commits into
tidyverse:masterfrom
thomasp85:label-tag

Conversation

@thomasp85

@thomasp85 thomasp85 commented Jan 17, 2018

Copy link
Copy Markdown
Member

As per discussions on twitter, this PR adds a tag label alongside title, subtitle, and caption. It is placed in the upper left corner in its own row and column between the margin and the title/x-label.

The tag name is chosen over label as the labs() function indicate that “label” is a catchall term for all types of titles.

The default theming is like this:

ggplot(mtcars) +
  geom_point(aes(disp, mpg)) + 
  labs(tag = 'A', title = 'test')

tag

The size is slightly larger than the title and the margin is chosen so that it is well separated from the main parts of the plot. In general this is based on my own taste and is up for discussion.

@thomasp85 thomasp85 requested a review from hadley January 17, 2018 19:32
@clauswilke

Copy link
Copy Markdown
Member

Thomas,

thanks for spearheading this. I see label positioning similar to legend positioning: There could be a position option in the theme that can either be a string such as "top-left", "bottom-left", etc, or a vector that takes coordinates. In the first case (string), you'd make a separate cell in the gtable. In the second case (coordinates), you'd just place the label wherever the coordinates request, allowing for placement anywhere in the entire plot area.

@hadley

hadley commented Jan 17, 2018

Copy link
Copy Markdown
Member

What if the label is longer? Does it overlap with the plot area, or does the margin expand?

@clauswilke

Copy link
Copy Markdown
Member

Hadley,

My personal opinion is that labels should generally be pasted onto the plot without expanding any margins. That almost always produces the best visual results. However, the API I proposed, which follows the legend.position example, allows for both approaches. Expand margins if a named position is provided and don't expand margins if coordinates are provided.

Best,
Claus

@thomasp85

Copy link
Copy Markdown
Member Author

I just had a similar discussion with @baptiste on twitter where I ended up proposing something similar to @clauswilke. My personal feeling is that the default should never allow overlapping of plot elements

@thomasp85

Copy link
Copy Markdown
Member Author

I’ll update this pr to include a more flexible positioning and we can take it from there

@baptiste

Copy link
Copy Markdown
Contributor

oops sorry was on my phone and limited to twitter. I agree with the general consensus; my personal view is that

  • 'tags' are 1 or 2-letter max (otherwise it's really a title in disguise), possibly with an extra character or two: little risk of terrible collisions
  • tags within the plot panel can be useful but are already straight-forward with annotate('label', x=-Inf) and co.
  • whenever I use tags in practice, I really want a tight fit because of space constraints; the unused top-left cell is typically a good spot already. If they were to add an extra cell unconditionally I'd probably never use them.
  • aligning tags across multiple plots on a device shouldn't be too much of an issue regardless of the extra cell, provided the justification is smart. If the tag is placed to span the whole gtable, it would need top-left justification to stick there when panels are aligned, whereas with the extra cell strategy the justification would happen within that cell and could be more flexibly tuned.

@thomasp85

Copy link
Copy Markdown
Member Author

I've added a plot.tag.position that behaves quite like legend.position (except it can target corners as well)

If you want a tight fit then provide coordinates, if you want automatic expansion to make room, use a string.

ggplot(mtcars) +
  geom_point(aes(disp, mpg)) + 
  labs(tag = 'A', title = 'test') +
  theme(plot.tag.position = "topleft") # Default

tag3

compared to using coordinates

ggplot(mtcars) +
  geom_point(aes(disp, mpg)) + 
  labs(tag = 'A', title = 'test') +
  theme(plot.tag.position = c(0, 1))

tag2

@thomasp85

Copy link
Copy Markdown
Member Author

Does the current implementation/features satisfy all parties?

@baptiste

baptiste commented Jan 19, 2018 via email

Copy link
Copy Markdown
Contributor

@clauswilke

Copy link
Copy Markdown
Member

I think this is all fine.

Do I understand correctly that this code should produce a tag that is left-aligned with the y axis title?

ggplot(mtcars) +
  geom_point(aes(disp, mpg)) + 
  labs(tag = 'A') +
  theme(plot.tag.position = c(0, 1),
        plot.tag = element_text(margin = margin(l = 5.5), hjust = 0))

@bhive01

bhive01 commented Jan 19, 2018

Copy link
Copy Markdown

The only thing I'm wondering about is tag length. As was mentioned on Twitter (and above), these things should be short. Are you going to restrict their length at all? I didn't see anything in the pull (may have missed it?).

@clauswilke

Copy link
Copy Markdown
Member

I've played around with the code a bit, and I can't say that I find the behavior of hjust, vjust, and margin intuitive.

  1. Setting hjust=0 makes the tag disappear:
ggplot(mtcars) +
  geom_point(aes(disp, mpg)) + 
  labs(tag = 'A', title = 'test') +
  theme(plot.tag.position = c(0, 1),
        plot.tag = element_text(hjust = 0))

screen shot 2018-01-19 at 10 22 17 pm

  1. Setting hjust = 1 places the tag in the middle of the plot:
ggplot(mtcars) +
  geom_point(aes(disp, mpg)) + 
  labs(tag = 'A', title = 'test') +
  theme(plot.tag.position = c(0, 1),
        plot.tag = element_text(hjust = 1))

screen shot 2018-01-19 at 10 22 26 pm

(The same is seen for vjust = 0 and vjust = 1 in the vertical direction.)

  1. Setting the top or left margin doesn't seem to have any effect:
ggplot(mtcars) +
  geom_point(aes(disp, mpg)) + 
  labs(tag = 'A', title = 'test') +
  theme(plot.tag.position = c(0, 1),
        plot.tag = element_text(margin = margin(t = 10, l = 10)))

screen shot 2018-01-19 at 10 22 32 pm

My expectation would be that hjust and vjust move the label relative to the tag position, as in (for example) geom_text() or geom_label(), and that margin would place a margin around the text (similar to label.padding in geom_label()).

@thomasp85

Copy link
Copy Markdown
Member Author

Yeah, I’ve stumbled onto that as well - I think the grob needs to be wrapped inside another gtree but I won’t get to it before monday

@thomasp85

Copy link
Copy Markdown
Member Author

@bhive01 I have no intention of limiting the number of characters in the tag - I cannot see any compelling reason to put an arbitrary limit on it...

@clauswilke I've played around with making justification work along with margins to no avail - I'll try some more but there're some grid interplay that complicates it...

@thomasp85

Copy link
Copy Markdown
Member Author

@clauswilke I have fixed the margin/justification interplay so that it now works as I would expect - can you test and see if it is sensible to you..?

@thomasp85

Copy link
Copy Markdown
Member Author

@hadley do you think this PR will get merged into the next release (if up to standard)? It's a rather fundamental addition in the 11th hour, but patchwork would like it :-)

@hadley

hadley commented Jan 22, 2018

Copy link
Copy Markdown
Member

Yes - I now thing implementing tidy evaluation isn't going to be that hard, so I'm going to hold off the release until that's in.

@clauswilke

Copy link
Copy Markdown
Member

I took a look at it. I think it's mostly working as expected but there's one remaining issue. The coordinate system doesn't seem to cover the entire plot area but instead the area minus the margin. I think this results in somewhat unintuitive behavior. First, with a tag position of (0, 1), the tag doesn't end up in the top left corner if we set hjust = 0 and vjust = 1 as one might expect:

p <- ggplot(mtcars) +
  geom_point(aes(disp, mpg)) + 
  labs(tag = 'A', title = 'test') +
  theme(plot.tag.position = c(0, 1))

p + theme(plot.tag = element_text(hjust = 0, vjust = 1))
p + theme(plot.tag = element_text(hjust = 0.5, vjust = 0.5))
p + theme(plot.tag = element_text(hjust = 1, vjust = 0))

screen shot 2018-01-22 at 4 16 26 pm

screen shot 2018-01-22 at 4 16 43 pm

screen shot 2018-01-22 at 4 16 59 pm

Second, while the middle result (with hjust = 0.5, vjust = 0.5) seems to look fine in this case, it actually behaves strangely when we use a longer tag:

ggplot(mtcars) +
  geom_point(aes(disp, mpg)) + 
  labs(tag = 'ABCDE', title = 'test') +
  theme(plot.tag.position = c(0, 1),
              plot.tag = element_text(hjust = 0.5, vjust = 0.5)         

screen shot 2018-01-22 at 4 26 52 pm

I'd rather be able to actually use the true top left corner of the plot as reference point and use the margins of the plot.tag element to match any plot margins if necessary. But this might be a matter of opinion. It's probably workable either way.

@thomasp85

Copy link
Copy Markdown
Member Author

Omitting the margin was intentional, but an easy change. The philosophy being that the margin is an actual empty space around the plot and if you want plot elements to extend to the edge you set the margin to zero.

I’m not going to be a user of freely positioned tags so I can be convinced to change it by a majority vote - what are the thoughts of @hadley, @baptiste, and @bhive01?

@clauswilke

Copy link
Copy Markdown
Member

I thought about it some more. I think it's fine as is. Placing the tag with plot.tag.position = c(0, 1), plot.tag = element_text(hjust = 0, vjust = 1) looks exactly right with my themes. I think the idea of having a true margin makes sense. And if somebody truly wants the label further to the plot boundary they can specify a position outside the 0--1 range.

@bhive01

bhive01 commented Jan 24, 2018

Copy link
Copy Markdown

I'm with @clauswilke on this. I finally got a chance to play with it (thanks dev_mode()!) and I understand what the position parts mean. All of the pieces including position and hjust/njust work how I expect so I'm 👍. Good inclusion for the needed multi-plot labeling problem.

@baptiste

Copy link
Copy Markdown
Contributor

Returning to a computer and playing with this last night I realised I didn't understand the design of titleGrob (namely the interaction of margins and just – I would have shifted the whole label+ margins block). With these existing conventions the tag implementation seems good to me (though there is I believe some slight inconsistency in handling justification between manual position vs added cells that may cause some surprises).

I can see why the plot margin might be seen as an extra margin around everything, but on the other hand it could be convenient to have a way to add something (not necessarily a tag) over the entire plot in npc, for instance a watermark/copyright/draft notice, or banners à la fivethirtyeight, etc. Perhaps it'd be best implemented as a separate item such as labs(annotation = element_custom(grob = textGrob('DRAFT', rot=45, cex=10))).

I'm also still wondering if it could make sense at some stage to let element_zzz() (and, ideally, other components) override their default position; the tag implementation would then default to t=0, l=0, etc.

Anyway, this is just a digression. I haven't had a chance to try it but I would still strongly recommend a comprehensive visual test in many configurations (font size, layout) and various themes. I remember being quite surprised at how bad some things looked with a large font size (as used in presentations/posters).

@karawoo karawoo modified the milestone: ‘«≤µ‘« Jan 25, 2018
@karawoo

karawoo commented Jan 25, 2018

Copy link
Copy Markdown
Member

(ignore that milestone, a keyboard cat was involved 😹)

@thomasp85

Copy link
Copy Markdown
Member Author

@hadley are you needing anything more for this PR (except time to review it)

@clauswilke

Copy link
Copy Markdown
Member

Since this hasn't been merged yet, I wanted to propose one very minor modification. I think the default size of the tags should be rel(1.2) rather than rel(1.3) as it currently is (see here). I'm proposing rel(1.2) because that is the size of the font used for the plot title. I think it's good typographic practice to try to limit the number of different font sizes used, and the current themes already have the problem (in my opinion) that they use too many different sizes (rel(0.8), rel(0.9), rel(1.0), rel(1.2)). Let's not make this problem worse. In fact, I have a pull request pending that cleans this up a bit and uses only three default font sizes, see here.

@baptiste

Copy link
Copy Markdown
Contributor

I agree, the font sizes are already confusing; I never realised a theme_bw(base_size=12) would lead to some of the text having larger font size such as rel(1.2). I'm currently answering a very unhappy referee who rated our manuscript of the lowest scholarly presentation; one complaint is

the panel names (a), (b) are extremely large and it is unusual to have titles above panels

(said names are set in 13pt, though the one-column draft preview blew up the whole figure).

@thomasp85

Copy link
Copy Markdown
Member Author

Thanks Lionel. I’ll look into where I’ve inadvertently passed a null to a grid call

@thomasp85

Copy link
Copy Markdown
Member Author

That was one weird bug grid had back then... 😱

should be fixed now for all versions

@clauswilke

Copy link
Copy Markdown
Member

Oh, that was the issue. It's the reason why I increased the minimum R version for cowplot to 3.3. Maybe you could add a small comment explaining why the code is written this way? E.g., "avoid assigning into unit vectors for compatibility with R <= 3.2."

@thomasp85

Copy link
Copy Markdown
Member Author

Sure - that's a good idea

@baptiste

baptiste commented May 9, 2018

Copy link
Copy Markdown
Contributor

If it's a new feature of grid, it's a shame to write code that is less transparent just to work around it for earlier versions of R, which in the future will be less and less relevant.
From what I vaguely recall this wasn't so much a bug as a missing [ method in grid? If so, couldn't ggplot2 add a private copy of this method conditional on R version <=3.2? (maybe in a specific file future.R collecting such temporary workarounds).

@thomasp85

Copy link
Copy Markdown
Member Author

I'll let such infrastructure decisions be up to @hadley - I don't personally think the new code is much less transparent as to warrant such measures...

@lionel-

lionel- commented May 9, 2018

Copy link
Copy Markdown
Member

@thomasp85 if you rebase on master the appveyor build should now be fixed, just in case you'd like to have that sweet green checkmark ;)

@hadley

hadley commented May 9, 2018

Copy link
Copy Markdown
Member

@thomasp85 Do you want to use justify_grob() now that the PR has been merged?

@thomasp85

Copy link
Copy Markdown
Member Author

Already implemented in 12646f8

@thomasp85

Copy link
Copy Markdown
Member Author

@lionel- it seems AppVeyor and vdiffr are still acting up despite a rebase...

@lionel-

lionel- commented May 9, 2018

Copy link
Copy Markdown
Member

@thomasp85 you should now use expect_doppelganger() without a namespace prefix, there is an indirection defined in helper-vdiffr.R. This way the visual test devolves to ggplot_build() on Appveyor (until I have a chance to finish adapting the new vdiffr version for Windows).

@thomasp85

Copy link
Copy Markdown
Member Author

gotcha

@hadley hadley merged commit 9cefab7 into tidyverse:master May 9, 2018
@lock

lock Bot commented Nov 5, 2018

Copy link
Copy Markdown

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock Bot locked and limited conversation to collaborators Nov 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants