Skip to content
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

dplyr release 1.0.1 breaks simple ggvis demo: layer_lines and layer_points produce no output. #5456

Closed
timcoote opened this issue Aug 3, 2020 · 4 comments

Comments

@timcoote
Copy link

@timcoote timcoote commented Aug 3, 2020

I realise that ggvis is in limbo, but I, and presumably others, have some shiny code in end devices that's not easy to update and it's taken a while to confirm that there is an issue and near where to place it.

So I thought that I'd at least document the issue, and potential fix, here. I first found a mention here: https://bit.ly/3k5KO2w.

From an engineering point of view, there may be some sort of regression as the failure is very quiet.


ggvis layers stop working. I'm afraid that I couldn't get reprex to work with rstudio-server, so I've put some code below.

install.packages ("ggvis")
library (magrittr)
library (ggvis)

mtcars %>% ggvis (x=~wt, y=~mpg) %>% layer_points()

I'd expect this to produce the "usual" plot. However, there are no points.
Untitled 3

However, this does work:

require (devtools)
install_version("dplyr", version = "0.8.5")
# <cr> over which versions to update
# I'm using RStudio Server. So at this point I restart R
library (magrittr)
library (ggvis)

mtcars %>% ggvis (x=~wt, y=~mpg) %>% layer_points()

And the plot with points appears.
Untitled 2

I did try to git bisect between 0.8.5 and 1.0.0, but the commit for 0.8.5 didn't seem to be in the clone from github, so I couldn't.

@timcoote
Copy link
Author

@timcoote timcoote commented Aug 5, 2020

I did manage to git bisect this from an earlier commit that's in the repo. git bisect log:

# bad: [90b3e4ea164514b57c72954c89dc63ab958eceb0] Bullet errors for summarise() (#4729)
git bisect bad 90b3e4ea164514b57c72954c89dc63ab958eceb0
# bad: [90b3e4ea164514b57c72954c89dc63ab958eceb0] Bullet errors for summarise() (#4729)
git bisect bad 90b3e4ea164514b57c72954c89dc63ab958eceb0
# good: [689518b4eb6adcfdeb5917dd97d41419b3e3d69b] Improve arrange docs
git bisect good 689518b4eb6adcfdeb5917dd97d41419b3e3d69b
# good: [3d45972610542138a790d8c1cd00a5990c0a1304] Upgrade tidyselect (#4720)
git bisect good 3d45972610542138a790d8c1cd00a5990c0a1304
# good: [cab26df184fa7032a16faa602fc529542b508be1] Remove unnused match_vars() function
git bisect good cab26df184fa7032a16faa602fc529542b508be1
# good: [dc8ebd4cb07c61a5e8fb9648a87fbf2f4628897a] Fix broken test (#4743)
git bisect good dc8ebd4cb07c61a5e8fb9648a87fbf2f4628897a
# good: [f7f66821327ce41f9a9a49bfc0f61434039f864f] Tweak error message
git bisect good f7f66821327ce41f9a9a49bfc0f61434039f864f
# bad: [887d40bf95cfe8c526245bd61fcde2c18af65e9d] Set tibble version (#4745)
git bisect bad 887d40bf95cfe8c526245bd61fcde2c18af65e9d
# bad: [125d75d6f76c78c27da45adc258f0b8b6a4620df] Simplify group metadata (#4728)
git bisect bad 125d75d6f76c78c27da45adc258f0b8b6a4620df
# first bad commit: [125d75d6f76c78c27da45adc258f0b8b6a4620df] Simplify group metadata (#4728)

There's quite a bit in that 'first bad' commit.

@lionel-
Copy link
Member

@lionel- lionel- commented Aug 7, 2020

Fixed in rstudio/ggvis#485

Install the branch with:

remotes::install_github("lionel-/ggvis@fix-dplyr-1-0-0")

But I think we should also fix this in dplyr. groups() should return NULL when groups are empty, as before.

@timcoote
Copy link
Author

@timcoote timcoote commented Aug 8, 2020

Thanks. I finally confirmed that this does work.

It's not really my business, but, personally, I'd make the new test more explicitly based on a boolean value, rather than a function returning a value of 0. If I could test it, I'd propose a change, but I can't - and you should ignore my comment if I'm writing in the face of the project style guide.

I also think that it is a better api to return an empty list if there are no groups, rather than null. R is weakly typed language, so it's not possible to have things like MAYBE types of Haskell to catch errors of not checking for changes in returned type.

@lionel-
Copy link
Member

@lionel- lionel- commented Aug 10, 2020

so it's not possible to have things like MAYBE types of Haskell

It is of course possible. There is a whole dynamically typed language funded on this kind of option type: lisp and nil punning.

I agree it is better to return list() in this case, but thought it wasn't worth the breaking change. What I missed is that dplyr would previously return NULL or list() interchangeably, making the return type complex. So the new behaviour was worth the change.

@lionel- lionel- closed this as completed Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants