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

.desc is not == to desc #174

Closed
jthomasmock opened this issue Feb 13, 2019 · 1 comment

Comments

@jthomasmock
Copy link

commented Feb 13, 2019

Hi team,

This is mainly related to my inability to read my own code, but I spent a while stuck on it as there wasn't a printed warning, issue or error. I believe this is a side-effect of the ... in forcats::fct_reorder().

I'd like to see a warning message along the lines of:

It looks like you used "desc" instead of ".desc" - desc will be ignored

or

It looks like you used "fun" instead of ".fun" - fun will be ignored

I imagine this could be an issue in other functions where .foo is also used - my apologies, I don't know what to call this...

### Example Reproducible Code

I am using a sort with summarize as an example for reproducibility, but the behavior popped up with plotting and facets where the same behavior can be observed (silent error of not descending sorting).

When adding arguments that begin with . such as .fun and .desc - if you drop the . you don't get an error but rather a silent reversion to the default there.

See below - putting desc instead of .desc leads to a "silent error" where the factors are sorted by the default (.desc = FALSE). Similarly, the .fun call can also be accidentally dropped.

mtcars %>% 
  rownames_to_column() %>%
  mutate(brand = word(rowname, 1),
         # putting `desc` instead of `.desc` leads to a silent error
         # the data is sorted by default .desc = FALSE
         brand = fct_reorder(brand, mpg,, fun = mean, desc = TRUE),
         cyl = factor(cyl)) %>%
  group_by(brand) %>% 
  summarize(mean = mean(mpg))

# A tibble: 22 x 2
   brand     mean
   <fct>    <dbl>
 1 Cadillac  10.4
 2 Lincoln   10.4
 3 Camaro    13.3
 4 Duster    14.3
 5 Chrysler  14.7
 6 Maserati  15  
 7 AMC       15.2
 8 Dodge     15.5
 9 Ford      15.8
10 Merc      19.0
# ... with 12 more rows

Alternatively, when you include the . in the .fun and .desc calls the anticipated behavior is observed, and the factors are correctly arranged.

mtcars %>% 
  rownames_to_column() %>%
  mutate(brand = word(rowname, 1),
         # .desc correctly called here, so factors are sorted as intended
         brand = fct_reorder(brand, mpg, .fun = mean, .desc = TRUE),
         cyl = factor(cyl)) %>%
  group_by(brand) %>% 
  summarize(mean = mean(mpg))

# A tibble: 22 x 2
   brand    mean
   <fct>   <dbl>
 1 Honda    30.4
 2 Lotus    30.4
 3 Fiat     29.8
 4 Toyota   27.7
 5 Porsche  26  
 6 Datsun   22.8
 7 Volvo    21.4
 8 Mazda    21  
 9 Hornet   20.0
10 Ferrari  19.7
# ... with 12 more rows

Plotting Unexpected Behavior

You can see the same behavior in ggplot2 when plotting with facets, the desc argument is also dropped silently.

### Ascending MPG
asc_mtcars <- mtcars %>% 
  rownames_to_column() %>%
  mutate(brand = word(rowname, 1),
         brand = fct_reorder(brand, mpg, desc = TRUE),
         cyl = factor(cyl)) %>% 
  ggplot(aes(x = cyl, y = mpg, group = cyl, color = cyl)) +
  geom_jitter() +
  facet_wrap(~brand) +
  labs(title = "Cars arranged by ascending mpg, used desc")

As opposed to intended behavior with .desc:

### Descending MPG
desc_mtcars <- mtcars %>% 
  rownames_to_column() %>%
  mutate(brand = word(rowname, 1),
         brand = fct_reorder(brand, mpg, .desc = TRUE),
         cyl = factor(cyl)) %>% 
  ggplot(aes(x = cyl, y = mpg, group = cyl, color = cyl)) +
  geom_jitter() +
  facet_wrap(~brand) +
  labs(title = "Cars arranged by descending mpg, used .desc")

@jthomasmock jthomasmock changed the title .desc is not = to desc .desc is not == to desc Feb 13, 2019

@hadley

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

This seems like a good place to use the ellipsis package:

library(forcats)
x <- fct_reorder(iris$Species, iris$Sepal.Width, desc = TRUE)
#> Warning: Some components of ... were not used: desc

Created on 2019-02-14 by the reprex package (v0.2.1.9000)

Getting the warning you suggested would require more work, and I'm not yet sure if this approach is going to work well in general, so at some point we'll need to revisit the warning message, but I think this is already a substantial improvement.

@hadley hadley closed this in 74830eb Feb 14, 2019

hadley added a commit that referenced this issue Feb 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.