search

tidyverts / tsibble

Tidy Temporal Data Frames and Tools
https://tsibble.tidyverts.org
GNU General Public License v3.0
528 stars 50 forks source link

Concerns about as_tibble.grouped_ts #224

Closed TylerGrantSmith closed 3 years ago

TylerGrantSmith commented 3 years ago

Diving deeper into #211 as suggested in https://github.com/tidyverse/dplyr/issues/5541#issuecomment-701419867_ the implementation should be reconsidered.

Unfortunately, naively using grouped_df(vec_data(x), vars = group_vars(x)) breaks 3 tests (2 in "filter() and slice() with .preserve = TRUE" and 1 in "select() with group_by()". I don't immediately see how to fix these issues without breaking other tests.

earowang commented 3 years ago

Thanks for looking into it.

https://github.com/tidyverts/tsibble/blob/c6e343d63eda12732cb3a6fc0205e4a0c85b871f/R/as-tsibble.R#L515-L518 The current implementation is basically the same as dplyr::new_grouped_df(). grouped_df() re-computes the group indices each time, which is inefficient (sometimes unnecessary) and causes unit tests to fail.

I think the issue for subsetting grouped_ts with 0 falls into {dplyr}.