Merge pkg into masters

[DavZim]

No description provided.

[DavZim]

Somehow I cannot push my local master to the repository. Is it possible that you merge the branches in this PR?

[krlmlr]

Can you push your local master under a different name, so that we can sort it out? Some changes from this branch are in conflict with the main master.

# Make sure working copy is clean
git checkout -b master-davzim master
git push -u origin HEAD

To bring local master in sync with this master, you can then do:

# Make sure working copy is clean
git checkout master
git reset --hard origin/master

This assumes that the remote that corresponds to this repo is called origin, adapt as necessary. (Sometimes it's called upstream or perhaps gadenbuie .)

[DavZim]

I have tried that but I am not sure if that was correct. The merge has also introduced old files from the master branch.
Currently the pkg branch consists of the package version including working joins, sets, and tidyr functionality. I.e., renaming (?) pkg as master should do the trick.

[krlmlr]

The master-davzim branch looks like master where the pkg branch has been merged into. I see two approaches now:

1. Open a PR from master-davzim to master, this should be mergeable without conflicts and show the relevant differences. We can review and double-check if the changes are intended.

2. Backport the missing commits from master into pkg (this might be the easier approach if you can confirm that most of these commits are already included in pkg). Below is the output of git log origin/pkg..origin/master --oneline:

   fe4d4b0 (origin/master, origin/HEAD, krlmlr/master, master) Add CODEOWNERS
   f92a350 Fix Fix tidyr spread/gather static image #9 by removing tidyr spread/gather grouped image
   69cb816 Remove border and use width = height = 0.9 for tiles
   adad81e Merge pull request Readme updates #8 from gadenbuie/readme-updates
   01f9450 Close tidyr :: spread() and gather() #1 with PR Add tidyr::spread and tidyr::gather #7 add tidyr spread and gather
   e2011de Requires cowplot
   a6dfac1 📚 Add text to readme for each section
   4304ec6 Re-order sections (background/learning up top)
   7848969 Add links to level 2 headers
   949d9ce Add tidyr::spread and tidyr::gather

[DavZim]

I am a bit unsure how to resolve this exactly.
We have worked on the pkg branch and neglected the master branch until the pkg was finished...
Now the pkg branch replaces the master branch entirely. That is, the changes and commits in the master branch that are not in the pkg branch are not needed anymore, with the exception of fe4d4b0.

The master-davzim branch is obsolete I think, that is, the pkg branch holds everything we need.

[krlmlr]

Then:

# Make sure working copy is clean
git checkout pkg
git cherry-pick --no-edit fe4d4b0
git merge master -X ours
git push

This will allow merging this PR.

[DavZim]

That looks good! (On a sidenote: Thanks for the help and your time!)
But we are still depending on a code review right?

[krlmlr]

You're welcome.

The technical challenges are resolved, we need approval from Garrick now.

[gadenbuie]

Sorry all that it's taken me a while to get to this. I had quite a bit going on around our recent holiday and didn't have time to get to this. I should be able to move this forward this week.

Personally, I prefer the master/dev branch style (along the lines of this git branching model), where the master branch contains complete features and user-ready code. In this case I was treating pkg as the dev branch.

Before we merge pkg to master, I'd like to survey the current state of the package branch to get a better sense of what's working and what's not quite ready yet. I'm happy to do this, but it might take me a little bit so @DavZim if you'd like to help out that would be appreciated.

[DavZim]

No worries, my simulations are running in the background so I have some time to look into this.

My conclusion is below. I also tweaked some minor things, added more options (color and sizes of cells), and updated both the Readme and the images

The status quo (branch master) allows the following functions:

• mutating and filtering joins
• set operations
• tidy data operations

each function is script based and does not allow for different datasets (i.e., other variable names etc) as parameters are hard coded.

This branch (branch pkg) refactors the code and introduces the package layer, while keeping the functionality. I.e., the same functions can be animated with similar results, the differences come from small design choices.

Furthermore, we introduce the following features in pkg:

• similar to the underlying functions tidyverse functions, we use rlang for non-standard evaluation, increasing the usability (e.g., to animate the base function left_join(df1, df2, by = "var1") the call becomes animate_left_join(df1, df2, by = "var1")). Where possible all functionality of the base functions is supported (see below for some examples where its not working).
• using anim_options() the user gains the freedom to specify many small animation details such as font size, family, or animation speed. The options can be set for the current session using the functions set_font_size() and set_anim_options(). If only one animation/pictures should be affected by the option, the user can choose to use the anim_opts = anim_options(...) argument or directly supply the options (animate_left_join(..., cell_width = 2)). The full list of possible options is described in ?anim_options (including the changes from my last commits about colors and cell sizes).

The animations/pictures for the users that do not wish to compile the graphics by themselves is updated to contain the new versions of the operations (in the folder images).

My conclusion, the pkg-branch is working good. Some user-facing information could be included in the readme to point the user to the features she can tweak, but our documentation in the package covers everything I found.
Another issue that we might want to address at one point, is that our errors are not telling enough, e.g., animate_gather(wide, key = "person", value = "sales") instead of animate_gather(wide, key = "person", value = "sales", -year) (missing -year), returns the error Error: Columns .key_map, .r, .col, .val, .type, .y, .header must be length 1 or 2, not 0, 0, 0, 0, 0, 0, 0 (might be a genuine error though).

So far missing is the functionality of named by-arguments in joins, e.g., the following does not work

left_join(band_members, band_instruments2, by = c("name" = "artist"))
animate_left_join(band_members, band_instruments2, by = c("name" = "artist"))

Whereas the following works as expected

band_instruments3 <- band_instruments %>% mutate(band = c("Beatles", "Beatles", "Stones"))

left_join(band_members, band_instruments3, by = c("name", "band"))
animate_left_join(band_members, band_instruments3, by = c("name", "band"))

[DavZim]

What is the current status of this PR, can we merge the branches or are we missing some functionality?

[coatless]

Just want to ping this PR to see the state.

I'd like to potentially scavenge some parts to automatically create figures for data instead of manually constructing figures (c.f. draw-r ). Are there any plans to move on this package formation soon (tm)?

[gadenbuie]

Overall, off the top of my head there are four main areas that require further attention:

1. When we wrote the code for the package version gganimate was still pre-release. There have been changes in the API since then that have not been incorporated into this package.

2. There are quite a few rough edges that need to be ironed out to make sure that the package works as advertised immediately after devtools::install_github(). More specifically, we need to clean up the namespace, move ggplot2 out of Depends and into Imports, and fill out documentation.

3. We need to straighten out fonts. I have some local code that uses the ggfittext package to handle font sizes, which is a really nice feature and removes a lot of the font size fiddling. Using newer packages like ragg might be a good idea, too. Personally I tend to use showtext + sysfonts for easy access to Google Fonts. Regardless the happy path for font usage needs to be worked out and consistent throughout the package or it causes headaches and cryptic error messages.

4. Documentation and tests. Ideally a pkgdown website that includes articles for each of the join styles (and any other action groups). This would provide a nice balance for users who just want to see/use the animations and others who would want to use the package.

Overall, I don't have a ton of any time to dedicate to work on this (although I would love to). Until the above are worked out, I don't mind that the package code lives in the pkg branch. It's now advertised from the master README and anyone who wants to experiment is welcome to check out and try the package version. Ideally, we'd make it out of the "experimental" stage and into the "maturing" stage before we move to master.

[gadenbuie]

@coatless You're welcome to scavenge here for anything relevant to your projects!