Vignette updates

[qdread]

Resolves #74 .

Here is a much improved vignette!

Summary of proposed changes:

• We now use the existing example datasets UCBAdmissions and vaccinations instead of toy data.
• The apps are embedded directly in the vignette using runtime: shiny.
• The code for both apps is much improved. I got rid of some hacky workarounds so that the code to extract data and find tooltip location is identical regardless of what the input data looks like.

Outstanding issues:

• A warning is generated for me when I run the vignette (see below). Not sure why this is.

Warning in file.create(to[okay]) :
  cannot create file 'C:\Users\qread\AppData\Local\Temp\RtmpEXKtRs\file3ce4269a7e8f_files/C:/Users/qread/Documents/R/win-library/4.0/rmarkdown/rmarkdown/templates/html_vignette/resources/vignette.css', reason 'Invalid argument'

• Every time I run the vignette an empty file vignettes/Rplots.pdf is generated.
• I left a placeholder at the end for referring the user back to where to look at the examples about wide and long data, not being sure what to put there.

[corybrunson]

Note to self: This commit must be installed in order for the system.file() command to find the new folders in inst/examples. : )

[corybrunson]

@qdread here are some thoughts on the updated vignette—some specific to the updates but others i might have made on the previous version. Overall, it is a great contribution, and thanks again for writing it! It's also meticulous, which i can't fully express how much i appreciate.

• When i render the vignette on my machine, the two shiny apps seem cramped: The graphics seem to have been scaled up beyond the window allocated for them. Maybe this only appears in some renderings, but ideally the apps should be readable and useable on any device. Here's a screenshot to illustrate, with the plot title partly cropped:

• In the first full paragraph under "Structure of the example app", which describes the steps in the app, could you (1) insert numbers, like this, before the various steps and (2) insert corresponding numbers in the character-string lines, or as new comment lines, in the pseudocode, so that it's easier for a reader to switch between the two?
• As a rule, i would avoid reassigning the name of a loaded data set, e.g. UCBAdmissions <- as.data.frame(UCBAdmissions). Maybe use the same name in a different format, e.g. snakecase: ucb_admissions. (I do something similar with Titanic in the examples.)
• The nodes are 1/3 wide by default, but this can be changed by passing a value to width.

The screenshots aren't rendering for me, but i'll have a look at that later, along with how to link to the other primary vignette toward the end.

[qdread]

Thanks for looking it over! All those comments should be easy to address -- I'll tweak the dimensions, add some kind of numbering to the steps, change the name of the ucb dataset, and modify the functions so that different widths can work. I think I misread the source code and thought that the width was hard-coded. I should be able to do all that sometime next week.

[corybrunson]

@qdread thank you, and do take your time. I won't be able to dig into this in earnest for another week or two.

[qdread]

Hi again - I took care of most of the issues.

Three things to still note:

1. I didn't fix the screenshots not displaying because it currently links to files on raw.githubusercontent.com which I assume will get updated when the PR is merged (?)
2. Still did not include links to the other vignettes at the end.
3. I reduced the size of the plot titles so they would be less likely to run off the display if the output is cramped. However I am not sure why the plot gets so cramped when you rendered it. I was unable to reproduce that. The things to tweak would be the width and height args on lines 42 and 366 of shiny.rmd, the height of the plot in pixels on line 91 of the Berkeley app.R (line 92 of the vax app.R), and the resolution of alluvialPlot on line 100 of the Berkeley app.R (line 101 in the vax one).

Let me know if you need me to mess with any of those, I just thought it might be more efficient for you to do it since you will be able to see the results of your tweaking.

[corybrunson]

Hi @qdread — i should be more available going forward!

I tried adjusting some sizes in the vignette and pushed the commit to vignette-updates-cory. Could you have a look and see if i broke anything on your end?

I'll make time to investigate the possibility of linking between vignettes ASAP.

[corybrunson]

Regarding inter-vignette hyperlinks, have you tried this approach? It's what i'll try when i come back to this!

[qdread]

I took a look at the updates on your branch and everything looks good. I think the only thing to possibly change, if you want, is to reduce the size of the geom_label() on line 25 of the UCBadmissions app and line 27 of the vaccinations app. With the smaller dimensions of the app window they now look too big. Maybe reduce to rel(2) instead of rel(3)? For me that looks much cleaner. Other than that all good!

The linking between vignettes looks simpler than I would have thought, too!

[corybrunson]

Hm. @qdread you're right about the labels, though it seems to be part of a more general problem that i'm not sure how to solve: The apps are much zoomed in on, so that all labels (and indeed line widths) are larger than i'd normally make them. Originally i hoped to fix the title overflow by reducing the apparent size this way, but i'm not sure how to go about it! I'll try the linking shortly.

[corybrunson]

@qdread i'm unable to install the package on my (old) machine, so i wonder if you could give the new commit a try. On the vignette-updates-cory branch, i made your suggested changes to the label sizes and added a link to the primary vignette modeled on the one at the StackOverflow answer above.

[qdread]

OK I'll give it a whirl shortly!

[qdread]

Hey I tested it and it seems to work well. I would suggest one more small tweak: previously, every time the vignette would render, it would create a file Rplots.pdf in the vignettes directory. I followed the advice at this SO post and added the line pdf(NULL) in the setup code block to suppress the file being generated. That might be a bit of a hack but seems to work. I wasn't sure if this was necessary to include in the final version. Is the Rplots.pdf only generated if the vignette is built interactively or will it be generated when users build the vignette locally? Not sure.

[qdread]

Also I don't think I can push to the vignette-updates-cory branch you created since it is not associated with this PR

[qdread]

Sorry for the multiple comments. I was also thinking about how the vignette with embedded app would work on the pkgdown site. I'm not sure it will work as is, because the pkgdown site has no R server to run the app. It would have to be hosted on a different server like shinyapps.io and embedded in the vignette that way. Check out this issue: https://github.com/r-lib/pkgdown/issues/838 which might be relevant.

[corybrunson]

@qdread oh whoops, how about i merge my subbranch into vignette-updates and push there? Do i have permission to?

Regarding the pkgdown site, i'll be glad to try the solution commented there, or to omit the vignette from the site generation (which i think can be done in the YAML header). Thanks for bringing it up.

FYI, since i'm thinking of it, i'll also need to address #71 before the next submission to CRAN.

[qdread]

Yeah, I think go ahead and merge to the vignette-updates branch on my fork, you should have permission.

[corybrunson]

Done! Good call. If it's OK with you, let's both just keep working on qdread:vignette-updates.

[corybrunson]

@qdread how are we on the vignette? If we're not too concerned with the sizing issue, then it may be ready to go.

For the next minor release (or even patch), i'll need to address #71 and #78. If data become available, i can also resolve #72.

The next major release will be the pivot branch, which updates the converter functions to better respect tidyr conventions (and some other things i don't remember right now). It will take a bit of additional work, but i think not much. It'd be great to have expert feedback on it, and it may also require some adjustments to your vignette.

So the question is whether the revised vignette should be part of a minor or major release. Do you have a strong opinion here? You can of course browse or install pivot to get a feel for the differences, or see NEWS.md in that branch for an overview (which i think is up-to-date).

[qdread]

Yeah I think it's more or less ready. I don't have a strong feeling about release -- it's no problem to wait a while until the major release. I can take a look at the pivot branch at some point soon to give feedback!

[qdread]

Hi again Cory, I am looking at the pivot branch. From what I can tell, the is_lodes_form and is_alluvia_form functions are convenience functions that are meant to be called by the user, not internally, so it should not affect the vignette, correct?

Overall I think the changes in the pivot branch look really nice. The one comment I have is that the arguments diffuse, distill, and discern of to_lodes_form are pretty cool. However it's a little confusing to understand exactly what they do. I'd recommend possibly clarifying the documentation a bit or adding more examples in the ex-alluvial-data.r -- especially for the diffuse argument. But again it looks great overall.

[corybrunson]

Hi @qdread and sorry for the delay—i'm still not quite ready to dive back into this.

The testing (is_*()) and conversion (to_*()) functions are indeed intended for users but they also are called by the stat layers. But their internal use has also been updated in pivot, so this change should affect only users' direct use of these functions.

That is a great comment about the to_lodes_form() parameters, thank you. I'm thinking that the parameters themselves might also be renamed, e.g. from diffuse to keep_axis_vars, from discern to make_axes_unique, and from distill to summarize_nonconstant_vars (or something shorter). Though that would disrupt the clever alliteration and alluvial theme, which i've found useful for thinking of them together (as correctives to the default pivoting procedure).

[corybrunson]

@qdread i encountered one additional problem with R CMD check, namely that the two uses of shinyAppDir() weren't recognized. It seems to be enough to explicitly search for them in the shiny namespace, i.e. shiny::shinyAppDir(). Do you not encounter this problem? If you don't, i'd like to understand why. If you do, then please just make the change and i'll merge the PR. : ) Thanks for your patience!

It looks like there will be no issues merging the Shiny vignette with the pivot branch, so we can get on that as soon as this step is done.

[qdread]

Hey sorry I saw your message a few days ago but forgot to follow up. I also got the error in R CMD check which was resolved by specifying the shiny namespace. Making the change now!