Vertical layout -- a different approach #111

matthewstern · 2021-02-02T05:08:37Z

This is inspired by #109 but takes a different approach, in which I adjust the overall flow of the finalize_plot() function to account for the new needs placed on it. My goal is to reduce redundancies to near zero and keep the function highly legible.

~~This is a work-in-progress. I'm trying to keep the commit descriptions clear, but I'll eventually update this post with a clear description about what this tinkers with.~~

EDIT: I think I have succeeded. This PR nets only a few new lines of code (not counting NEWS.md), but both adds features and eliminates redundancy. Detailed changes down below in this comment.

Rather than wrapping `prepare_chart()` in a tryCatch and setting aes defaults outside the function, move the default aes handling to inside the function. This actually allows for much cleaner code, as we can utilize the function `on.exit()` instead of `tryCatch()`.

removes the subfn `construct_layout`, reintegrating these components into the main function. Delays the call to `prepare_chart()` and creation of `vp.plotbox` until the plot is actually used. This is in preparation for delaying the calculation of plotbox size. Integrates the `prepare_chart` subfn directly into `grob_plot`.

and new calculation forplotbox height adjusting for the height of the new bottom caption bottom.

matthewstern · 2021-02-02T05:14:42Z

Current progress: No conditionals built in yet, but at the moment the bottom caption grob is in place and the plotbox adjusts accordingly. This seems to work very well where legend_shift = FALSE:

But something I haven't caught yet is oversizing the grob table where legend_shift = TRUE:

I haven't fully thought through the conditional yet but I think I'd like to leverage our experience that suggests the title and caption grobs functionally disappear when title_width = 0: we don't need to actually remove them. So, I believe that the only conditional we'll need will be adding in the bottom caption when title_width = 0 and adjusting the plotbox height calculation so it replaces grobHeight(grob_caption_bottom) with 0 when that grob does not exist.

`vert_mode` is, for the time being, interpreted from the existing `title_width` argument. grobs are now added to a list `grobs` rather than specified individually, so that the `final_plot` can be constructed dynamically (e.g. with or without unnecessary grobs) without complex code.

`grobs$title` and `grobs$caption` now only created in horizontal/traditional mode, while `grobs$caption_bottom` now only created in vertical mode. New `safe_grobHeight` subfn takes a list object (eg `grobs$this_grob`) and returns 0 bigpts if `this_grob` does not exist in list `grobs`. Otherwise, it returns `grid::grobHeight()`.

I'm not sure what was going on with the incorrect plot sizing when `legend_shift = TRUE`, but I traced the break back to when I had redefined plotbox_height as a complex height object that incldued some bigpts and the `grobHeight(grobs$caption_bottom)`. This commit modifies the `safe_grobHeight()` fn to convert `grobHeights` into other units (mainly, bigpts) and return them as unitless objects. This allowed me to revert to Daniel's original code for calculating the `plot_height` and generating the `built` object. This also allowed me to simplify the other code that refers to `grobHeight()`s.

simplifies some unnecessary initialization checks. Shifts the location of `margin_title_b` to, logically, the title grob rather than the caption grob. Adjusts documentation accordingly - both ?finalize_plot and the finalize vignette.

references to `final_plot` are now `finished_graphic` to clarify that the "plot" is the thing drawn in the "plotbox" -- the "graphic" is the assembly of grobs that represent the finished product.

`prepare_chart()` is now `prepare_plot()` to continue to align terms

matthewstern · 2021-02-21T01:45:35Z

This is ready for review and testing. This makes a variety of under-the-hood improvements to finalize_plot() that not only allow for below-plot caption placement but also clean up and streamline the function to pave the way for future implementations, such as tmaps. Here's a summary of changes:

the construct_layout() subfn has been eliminated, which means the creation of grobs now occurs in the main function again. The prepare_plot() subfn (used to be prepare_chart()) has been streamlined by incorporating error handling into the function. This is also now called much later in the function, when it's needed, to allow the plotbox height to be calculated later on.
plotbox height is now calculated after the caption grob is created. This allows for the plotbox to be drawn smaller as per the space needed for the caption.
there is a new variable specified in the function (vert_layout for now) that specifies whether the title+left caption or the bottom caption should be drawn. For now, this is determined behind the scenes, when title_width = 0 and does not require additional user input to trigger.
The already-existent title and left captions are now built only if vert_layout = FALSE, which allowed for the elimination of code that set title = "" when title_width = 0. The new bottom caption is only built if vert_layout = TRUE.
grobs are now built into a list. e.g. the object that used to be grob_title is now grobs$title. This, importantly, allows for an unspecified set of grobs to be easily drawn into the final_plot grobTree object.
new subfn safe_grobHeight() converts grobHeights to bigpoints (or anything else), and returns 0 of that grob doesn't exist, which substantially simplifies dimension specifications that rely on measurements of the heights of other grobs.
EDIT: deprecated (but did not remove) argument caption_valign in favor of new argument caption_align, which is numeric and sets the alignment for either a left or bottom caption. The shift to numeric utilizes the full functionality of the underlying function, e.g. enabling any decimal between 0 and 1 (e.g. 0.5 aligns center). This is the only user-facing/input change made by this PR and only known backwards compatibility concern.

There's enough change under the hood here -- and we now can more officially say we support no-sidebar graphics -- that I'm suggesting we update version to 1.1.0. But I'm flexible.

Sample outputs

(p is the code taken from the setup code chunk in the finalize vignette.)

finalize_plot(plot = p,
              title = "Annual unlinked passenger trips (in millions).",
              caption = "Source: Chicago Metropolitan Agency for Planning
                analysis of Regional Transportation Authority data.",
              title_width = 1.8,
              caption_align = 1,
              debug = TRUE)

finalize_plot(plot = p,
              title = "Annual unlinked passenger trips (in millions).",
              caption = "Source: Chicago Metropolitan Agency for Planning
                analysis of Regional Transportation Authority data.",
              title_width = 0,
              caption_align = 1,
              debug = TRUE)

to enable horizontal caption alignment, deprecate `caption_valign` in favor of new arg `caption_align` that takes a numeric 0 to 1 and is passed directly to left caption grob's `valign` argument and bottom caption grob's `halign` argument. This prevents the addition of a `caption_halign` argument, and more completely utilizes the underlying functionality `textbox_grob()`'s valign and halign arguments (ie user can now pass 0.5 for center alignment).

Minor changes

create news.md

nmpeterson · 2021-02-22T17:54:19Z

Just a reminder that we'll need to:

change the date for release 1.1.0 in NEWS.md whenever this PR gets merged.

Also, the old figure numbering was a complete mess

title_width must range from 0 to width/2. caption_align from 0 to 1.

Changes in capitalization and line breaks for consistency

dlcomeaux · 2021-02-24T02:31:54Z

R/finalize_plot.R

+    )
+  }
+
+  # in vertical mode, and if caption exists, create bottom caption box (but no title)


I understand the rationale for this - however, I think that it may be helpful in the future to have the ability to have a title in the vertical layout, potentially above the bar (which was what I had implemented, however jankily, in #109 ). It would create some more complexity, because then you need to have an adjustment to the top line positioning - although I think we could accomplish that with a similar use of the safe_grobheight function you've developed (that one is nice, by the way). Not an urgent function, but something to think about for the future.

Agreed. My apologies for missing that part of your work in #109. With safe_grobHeight() and the new conditional grob build framework, it shouldn't be hard to install an alternate title grob above the topline. It would be really easy, I think... if a new grob top_title (or maybe we change to title_vert and caption_vert) gets created before the topline does, the topline height could be set to the top margin plus safe_grobheight(top_title)...

If you wanted to put together a PR that (re)implements this, I don't see a reason not to implement it.

dlcomeaux

I made some minor formatting tweaks and have one suggestion for future functionality (adding titles to vertical layouts) which others may have different ideas on. I think that this is a great upgrade to the function, though - great work, @matthewstern .

dlcomeaux · 2021-02-24T02:58:07Z

@matthewstern @nmpeterson I'm not sure what has happened here - the Windows checks are failing on GitHub, but are not failing when I run them on my computer. The error seems to have something to do with a package called ggsignif in the installation (which is failing to install), but that could be incidental. Not sure what's up, since all my changes were in commented text.

This reverts commit 3bafdd1.

This reverts commit 998cdbf.

dlcomeaux · 2021-02-24T03:08:34Z

Hmm and it's not the result of my commit, since the check still fails when I reverted it. Not sure what's up.

matthewstern · 2021-02-24T15:49:38Z

@matthewstern @nmpeterson I'm not sure what has happened here - the Windows checks are failing on GitHub, but are not failing when I run them on my computer. The error seems to have something to do with a package called ggsignif in the installation (which is failing to install), but that could be incidental. Not sure what's up, since all my changes were in commented text.

@dlcomeaux, thanks for looking! I ran into a similar issue with the auto-checks a few PRs ago. It looks like the package ggsignif was published yesterday to 0.6.1, but that CRAN has not yet updated the package binaries. Last time the error we were getting was that the package was just not available at the version the check VM wanted. This error seems a bit more complex than that, but considering the version difference and recent update, I'm hopeful that's the problem. It seems to take CRAN a few days to do whatever it is they do.

Considering the Mac check passes (and Windows checks pass on both our computers, I think it's reasonable to merge this and re-check the master branch in a few days to verify no issues. Does that seem reasonable? (@nmpeterson)

EDIT: I love that the check is failing because of a package that enables significance brackets... so arcane.

nmpeterson · 2021-02-24T16:11:55Z

I'm re-running the jobs right now. If they fail again, I do think it's fine to force the merge. As @matthewstern noted, these issues can crop up occasionally when the virtual machines are setting up their R environment (which is done from scratch each time a job is run) and new versions of dependency packages have recently been added to CRAN but the compiled binaries for those packages are not yet ready.

Edit: aaaaand it failed.

matthewstern · 2021-02-24T17:13:40Z

FWIW the pkgdown build also failed after merging, due maybe to a similar issue in package "broom"? https://github.com/CMAP-REPOS/cmapplot/runs/1972107044

Hopefully both issues will resolve themselves in the coming days.

nmpeterson · 2021-02-24T17:17:47Z

It's pillar, actually. Note the CRAN page currently lists the version as 1.5.0 but the binaries are still 1.4.7. We can re-run the jobs when the binary versions match the package version.

matthewstern · 2021-02-24T17:19:34Z

:-(

matthewstern and others added 3 commits February 1, 2021 16:54

bottom caption grob

26496ca

and new calculation forplotbox height adjusting for the height of the new bottom caption bottom.

matthewstern changed the title ~~Vertical layout -- a different spin~~ Vertical layout -- a different approach Feb 2, 2021

Matthew Stern added 6 commits February 20, 2021 14:05

clean up

5588ae3

simplifies some unnecessary initialization checks. Shifts the location of `margin_title_b` to, logically, the title grob rather than the caption grob. Adjusts documentation accordingly - both ?finalize_plot and the finalize vignette.

advance version, improve nomenclature

36c1462

references to `final_plot` are now `finished_graphic` to clarify that the "plot" is the thing drawn in the "plotbox" -- the "graphic" is the assembly of grobs that represent the finished product.

additional nomenclature tweaks

2deb520

`prepare_chart()` is now `prepare_plot()` to continue to align terms

Merge branch 'master' into vertical-layout-2

078af49

matthewstern marked this pull request as ready for review February 21, 2021 01:48

matthewstern requested a review from dlcomeaux February 21, 2021 01:56

Matthew Stern added 3 commits February 20, 2021 22:06

create news.md

2c61932

roll back version

c698c52

matthewstern mentioned this pull request Feb 22, 2021

create news.md #112

Merged

Matthew Stern and others added 4 commits February 21, 2021 21:43

news tweaks

63da673

Update NEWS.md

17935b2

Update NEWS.md

525edaa

Minor changes

Merge pull request #112 from CMAP-REPOS/news

7b4e20a

create news.md

Noel Peterson added 4 commits February 22, 2021 12:22

More context for title-less graphs

d304eeb

Also, the old figure numbering was a complete mess

Markdown formatting

e35ee0b

Enforce min/max values for title_width and caption_align

96ab02a

title_width must range from 0 to width/2. caption_align from 0 to 1.

Replace caption_valign examples with caption_align

a5a41d7

nmpeterson approved these changes Feb 22, 2021

View reviewed changes

nmpeterson mentioned this pull request Feb 22, 2021

Vertical layout #109

Closed

nmpeterson added the enhancement New feature or request label Feb 22, 2021

Minor formatting tweaks

3bafdd1

Changes in capitalization and line breaks for consistency

dlcomeaux reviewed Feb 24, 2021

View reviewed changes

dlcomeaux approved these changes Feb 24, 2021

View reviewed changes

Line break

b102df3

dlcomeaux added 2 commits February 23, 2021 21:00

Revert "Minor formatting tweaks"

998cdbf

This reverts commit 3bafdd1.

Revert "Revert "Minor formatting tweaks""

21f1785

This reverts commit 998cdbf.

Update NEWS.md

83857de

matthewstern merged commit 34d805d into master Feb 24, 2021

matthewstern deleted the vertical-layout-2 branch February 24, 2021 16:59

dlcomeaux mentioned this pull request Feb 25, 2021

Add title to vertical chart layout #113

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vertical layout -- a different approach #111

Vertical layout -- a different approach #111

matthewstern commented Feb 2, 2021 •

edited

Loading

matthewstern commented Feb 2, 2021

matthewstern commented Feb 21, 2021 •

edited

Loading

nmpeterson commented Feb 22, 2021 •

edited by matthewstern

Loading

dlcomeaux Feb 24, 2021

matthewstern Feb 24, 2021

dlcomeaux left a comment

dlcomeaux commented Feb 24, 2021

dlcomeaux commented Feb 24, 2021

matthewstern commented Feb 24, 2021 •

edited

Loading

nmpeterson commented Feb 24, 2021 •

edited

Loading

matthewstern commented Feb 24, 2021

nmpeterson commented Feb 24, 2021 •

edited

Loading

matthewstern commented Feb 24, 2021

Vertical layout -- a different approach #111

Vertical layout -- a different approach #111

Conversation

matthewstern commented Feb 2, 2021 • edited Loading

matthewstern commented Feb 2, 2021

matthewstern commented Feb 21, 2021 • edited Loading

Sample outputs

nmpeterson commented Feb 22, 2021 • edited by matthewstern Loading

dlcomeaux Feb 24, 2021

Choose a reason for hiding this comment

matthewstern Feb 24, 2021

Choose a reason for hiding this comment

dlcomeaux left a comment

Choose a reason for hiding this comment

dlcomeaux commented Feb 24, 2021

dlcomeaux commented Feb 24, 2021

matthewstern commented Feb 24, 2021 • edited Loading

nmpeterson commented Feb 24, 2021 • edited Loading

matthewstern commented Feb 24, 2021

nmpeterson commented Feb 24, 2021 • edited Loading

matthewstern commented Feb 24, 2021

matthewstern commented Feb 2, 2021 •

edited

Loading

matthewstern commented Feb 21, 2021 •

edited

Loading

nmpeterson commented Feb 22, 2021 •

edited by matthewstern

Loading

matthewstern commented Feb 24, 2021 •

edited

Loading

nmpeterson commented Feb 24, 2021 •

edited

Loading

nmpeterson commented Feb 24, 2021 •

edited

Loading