Skip to content

Add tag label for adding identifier to plot #2405

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
May 9, 2018

Conversation

thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Jan 17, 2018

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
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
Copy link
Member

hadley commented Jan 17, 2018

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

@clauswilke
Copy link
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
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
Member Author

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

@baptiste
Copy link
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
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
Member Author

Does the current implementation/features satisfy all parties?

@baptiste
Copy link
Contributor

baptiste commented Jan 19, 2018 via email

@clauswilke
Copy link
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
Copy link

bhive01 commented Jan 19, 2018

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
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
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
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
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
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
Copy link
Member

hadley commented Jan 22, 2018

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
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
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
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
Copy link

bhive01 commented Jan 24, 2018

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
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
Copy link
Member

karawoo commented Jan 25, 2018

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

@thomasp85
Copy link
Member Author

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

@clauswilke
Copy link
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
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
Member Author

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

@thomasp85
Copy link
Member Author

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

should be fixed now for all versions

@clauswilke
Copy link
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
Member Author

Sure - that's a good idea

@baptiste
Copy link
Contributor

baptiste commented May 9, 2018

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
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-
Copy link
Member

lionel- commented May 9, 2018

@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
Copy link
Member

hadley commented May 9, 2018

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

@thomasp85
Copy link
Member Author

Already implemented in 12646f8

@thomasp85
Copy link
Member Author

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

@lionel-
Copy link
Member

lionel- commented May 9, 2018

@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
Member Author

gotcha

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

lock bot commented Nov 5, 2018

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