Closed qdread closed 3 years ago
Thanks @qdread! I am able to generate the vignette locally and will carefully read it soon. I'll check the terminology, but your other notes don't concern me too much.
Hi @qdread — i've had a look, and there are only a few points i want to address before merging:
shiny-vignette
branch or for me to make a checklist here of the suggestions for you to make and check off as done?It's exciting to be able to include this example with the package! Thanks a lot for going through the hassle and sharing what you learned.
P.S. The Shiny app runs just fine on my machine, too.
Hi, thanks for looking over everything! I'll respond more fully to your notes shortly (by early this coming week) but to answer your last question, you can go ahead and just make commits to the shiny-vignette branch on the fork qdread/ggalluvial. I think it is set so you have push access without having to make a PR.
To address more of your points:
group
column as the primary key then by the axis
columns. So that'd be necessary for other users to do, otherwise the y-coordinate will not refer to the correct row. Second, the one-to-one correspondence between row of data frame and y-axis position is only because all the weight
values are 1. If any weights were not 1, we would have to calculate the y-axis position by something like cumsum(weight)
. Do you want me to write the code to work for these more general situations?inst
folder with the source code, so the lengthy code is not reproduced in the vignette.So given all of that, there are a few possible modifications: making the code more general for unequal weights and wide vs. long format, changing the toy data to one of the real example datasets, and either embedding the app or moving the full source code to an external file.
If you want me to implement any of those changes, please let me know and I'll try to add more commits to the branch!
That is a great summary of options, thank you!
While i think the current version would be worth including in a new release ASAP, i think it would eventually be important to make most of these changes in future versions. It sounds from your description like the following would be possible:
My thinking here is that, at least in non-developer–targeted collections like tidyverse, R packages should make it very easy for users to find examples of things they might want to do, which they can then tinker with and adapt rather than write from scratch.
What do you think? If you feel that any of these are worth doing before the next release, then i'm happy to wait and help as able, but otherwise i can push the revisions i have in mind for the current vignette later today and we can release a new minor version later this week.
@qdread also, would you please add yourself to the DESCRIPTION as an author?
Hey I just added a small note to the text addressing the point that the data frame has to be sorted by group
for the y-axis not to get scrambled up, and I added myself as author (thanks by the way! 🙌 )
I agree that we can go ahead and release this now but I should definitely have time in the near future to work on the other points discussed above (wide vs long, unequal size
of each row, replace toy data with real example, embed shiny app and include example source code). Definitely bother me about it if I drop the ball and don't make any progress on it for a while!
@qdread sounds good! I pushed my formatting changes just now, and i restored the missing ID
column because its absence caused certain features of the Shiny app to malfunction. If the current version looks good to you, let me know and i'll do some final bookkeeping (checklist below & add anything you notice missing) and merge the pull request.
Thanks for doing the continued work! I'll be available (with varying expeditiousness) to help as needed along the way. I noticed again during proofreading the several particular choices made in this example that could be changed for additional examples (installed or vignette), e.g. strata and alluvia versus lodes and flows, hover versus click.
NEWS.md
with Shiny vignette summary.Suggests
.CITATION
with coauthored package citation.Whoops, thanks for adding that column back in. For some reason I thought it wasn't used and got rid of it but never tested it 🤦♂️ . But yes, the current version looks great -- Merge away! I'll be in touch later when I have a chance to work on it some more.
@qdread do you know if your vignette depends on any specific versions of shiny, htmltools, or sp? (Asking for DESCRIPTION.)
I had shiny 1.4.0.2, htmltools 0.4.0, and sp 1.4.4 installed locally when I built the vignette. That is the most recent version of sp but not the other two. I am not sure whether the vignette depends on those versions specifically, should I check?
Ideally (as i understand it), the oldest versions that still support the used functionality should be specified in the DESCRIPTION. Rather than nail down the exact oldest possible version, it would probably be OK to go back a few minor releases of each package, check that the app works as designed, and then specify those releases. I'm glad to try this too.
This section of Wickham and Bryan's book is my (only) point of reference.
Hey Cory, I rolled back shiny, htmltools, and sp a few versions. However when I tried to run the app, it automatically prompted me to update shiny (which also updated htmltools). So I think it is fine to keep the shiny and htmltools dependencies to the versions I listed above. But I was able to run the app with sp 1.0.0 so you can probably use that as the minimum version.
One final note that I realized while doing this is that the sessionInfo()
at the end of the app is not at all informative because we don't actually evaluate any of the R code that loads the packages. So it is not showing any of the versions that the user would care about. So I would recommend just omitting the sessionInfo()
from the vignette.
I tried some experiments on my own and couldn't get the app to work on older versions of sp. So, for safety, i specified a more recent version, along with one for shiny that's still fairly recent but also worked. Time to merge this PR, and i'll ping you when the release is submitted!
@qdread sorry for any confusion about the merge process. I wasn't sure that CI had been running on the PR so created a temp branch to merge into before main
. All good now, i think. : )
Whoops, I meant to say sp 1.2.5 was the version I successfully tested. (I got it confused with the Shiny version I tested). So if you want to reduce the version dependency to 1.2.5 it should work. Anyway thanks for merging my PR! It was fun to work on it.
This is a vignette summarizing what I learned from issue #68 !
Notes
shiny
,htmltools
, andsp
. Sorry for bloating up the dependencies if you decide to do that!