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

as.period fails for grouped data #930

Closed
julianbarg opened this issue Nov 2, 2020 · 2 comments
Closed

as.period fails for grouped data #930

julianbarg opened this issue Nov 2, 2020 · 2 comments

Comments

@julianbarg
Copy link

as.period fails with a nondescript error when applied to a grouped dataframe.

df <- data.frame(group = c("a","a","b"),
                 p1 = as.Date(c("2020-10-23", "2020-10-24", "2020-10-25")), 
                 p2 = as.Date(c("2020-10-26", "2020-10-27", "2020-10-28")))

#This succeeds
df %>%
  mutate(interval = as.period(interval(start = p1, end = p2)))

# This throws the error message
df %>%
  group_by(group) %>%
  mutate(interval = as.period(interval(start = p1, end = p2)))

Error message:
Error: Problem with mutate() input interval.
x Internal error in vec_proxy_assign_opts(): proxy of type double incompatible with value proxy of type integer.
ℹ Input interval is as.period(interval(start = p1, end = p2)).
ℹ The error occurred in group 2: group = "b".

@vspinu
Copy link
Member

vspinu commented Nov 5, 2020

@DavisVaughan you have added support for vectrs. Is this issue expected?

@DavisVaughan
Copy link
Member

I think this is a lubridate bug? I think that year/month/day/hour/minute components of a Period object should always be integer, and the second component should be numeric. But that is not the case. Creating a period object from as.period() maintains this property, but creating one from period() or a helper like days() does not. That causes the problem.

library(lubridate)
#> 
#> Attaching package: 'lubridate'
#> The following objects are masked from 'package:base':
#> 
#>     date, intersect, setdiff, union
library(vctrs)

x <- as.Date("2020-10-25")
interval <- interval(x, x + 1)
period_from_interval <- as.period(interval)

period <- days(3)

get_fields <- function(x) {
  list(
    year = x@year, 
    month = x@month, 
    day = x@day, 
    hour = x@hour, 
    minute = x@minute, 
    second = x@.Data
  )
}

# this feels correct
vapply(get_fields(period_from_interval), class, character(1))
#>      year     month       day      hour    minute    second 
#> "integer" "integer" "integer" "integer" "integer" "numeric"

# this feels wrong?
vapply(get_fields(period), class, character(1))
#>      year     month       day      hour    minute    second 
#> "numeric" "numeric" "numeric" "numeric" "numeric" "numeric"

# the shared empty period object is just created by:
# `delayedAssign("lubridate_shared_empty_period", period())`
vapply(get_fields(lubridate:::lubridate_shared_empty_period), class, character(1))
#>      year     month       day      hour    minute    second 
#> "numeric" "numeric" "numeric" "numeric" "numeric" "numeric"

# we expect the same types as `lubridate_shared_empty_period`, so this errors
vec_c(period_from_interval, period_from_interval)
#> Error: Internal error in `vec_proxy_assign_opts()`: `proxy` of type `double` incompatible with `value` proxy of type `integer`.

# this has the same type as `lubridate_shared_empty_period`, so no error
vec_c(period, period)
#> [1] "3d 0H 0M 0S" "3d 0H 0M 0S"

Created on 2020-11-05 by the reprex package (v0.3.0.9001)

Maybe these should be integers (except seconds)

lubridate/R/periods.r

Lines 449 to 450 in b9025e6

out <- list(second = 0, minute = 0, hour = 0, day = 0,
week = 0, month = 0, year = 0)

And day math here might should use 7L

out$day <- out$day + 7 * out$week

An even more robust way to handle this is probably to handle it in the initialization function for Period. Let validObject() first do its checks to make sure the values are integer-ish, then after that actually convert the components to integer

validObject(.Object)

@vspinu vspinu closed this as completed in e76abf7 Nov 10, 2020
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue May 30, 2021
Version 1.7.10
==============

### NEW FEATURES

* `fast_strptime()` and `parse_date_time2()` now accept multiple formats and apply them in turn

### BUG FIXES

* [#926](tidyverse/lubridate#926) Fix incorrect division of intervals by months involving leap years
* Fix incorrect skipping of digits during parsing of the `%z` format

Version 1.7.9.2
===============

### NEW FEATURES

* [#914](tidyverse/lubridate#914) New `rollforward()` function
* [#928](tidyverse/lubridate#928) On startup lubridate now resets TZDIR to a proper directory when it is set to non-dir values like "internal" or "macOS" (a change introduced in R4.0.2)
* [#630](tidyverse/lubridate#630) New parsing functions `ym()` and `my()`

### BUG FIXES

* [#930](tidyverse/lubridate#930) `as.period()` on intervals now returns valid Periods with double fields (not integers)



Version 1.7.9
=============

### NEW FEATURES

* [#871](tidyverse/lubridate#893) Add `vctrs` support


### BUG FIXES

* [#890](tidyverse/lubridate#890) Correctly compute year in `quarter(..., with_year = TRUE)`
* [#893](tidyverse/lubridate#893) Fix incorrect parsing of abbreviated months in locales with trailing dot (regression in v1.7.8)
* [#886](tidyverse/lubridate#886) Fix `with_tz()` for POSIXlt objects
* [#887](tidyverse/lubridate#887) Error on invalid numeric input to `month()`
* [#889](tidyverse/lubridate#889) Export new dmonth function

Version 1.7.8
=============

### NEW FEATURES

* (breaking) Year and month durations now assume 365.25 days in a year consistently in conversion and constructors. Particularly `dyears(1) == years(1)` is now `TRUE`.
* Format and print methods for 0-length objects are more consistent.
* New duration constructor `dmonths()` to complement other duration constructors.
*
* `duration()` constructor now accepts `months` and `years` arguments.
* [#629](tidyverse/lubridate#629) Added `format_ISO8601()` methods.
* [#672](tidyverse/lubridate#672) Eliminate all partial argument matches
* [#674](tidyverse/lubridate#674) `as_date()` now ignores the `tz` argument
* [#675](tidyverse/lubridate#675) `force_tz()`, `with_tz()`, `tz<-` convert dates to date-times
* [#681](tidyverse/lubridate#681) New constants `NA_Date_` and `NA_POSIXct_` which parallel built-in primitive constants.
* [#681](tidyverse/lubridate#681) New constructors `Date()` and `POSIXct()` which parallel built-in primitive constructors.
* [#695](tidyverse/lubridate#695) Durations can now be compared with numeric vectors.
* [#707](tidyverse/lubridate#707) Constructors return 0-length inputs when called with no arguments
* [#713](tidyverse/lubridate#713) (breaking) `as_datetime()` always returns a `POSIXct()`
* [#717](tidyverse/lubridate#717) Common generics are now defined in `generics` dependency package.
* [#719](tidyverse/lubridate#719) Negative Durations are now displayed with leading `-`.
* [#829](tidyverse/lubridate#829) `%within%` throws more meaningful messages when applied on unsupported classes
* [#831](tidyverse/lubridate#831) Changing hour, minute or second of Date object now yields POSIXct.
* [#869](tidyverse/lubridate#869) Propagate NAs to all internal components of a Period object

### BUG FIXES

* [#682](tidyverse/lubridate#682) Fix quarter extraction with small `fiscal_start`s.
* [#703](tidyverse/lubridate#703) `leap_year()` works with objects supported by `year()`.
* [#778](tidyverse/lubridate#778) `duration()/period()/make_difftime()` work with repeated units
* `c.Period` concatenation doesn't fail with empty components.
* Honor `exact = TRUE` argument in `parse_date_time2`, which was so far ignored.

Version 1.7.4
=============

### NEW FEATURES

* [#658](tidyverse/lubridate#658) `%within%` now accepts a list of intervals, in which case an instant is checked if it occurs within any of the supplied intervals.

### CHANGES

* [#661](tidyverse/lubridate#661) Throw error on invalid multi-unit rounding.
* [#633](tidyverse/lubridate#633) `%%` on intervals relies on `%m+` arithmetic and doesn't produce NAs when intermediate computations result in non-existent dates.
* `tz()` always returns "UTC" when `tzone` attribute cannot be inferred.

### BUG FIXES

* [#664](tidyverse/lubridate#664) Fix lookup of period functions in `as.period`
* [#649](tidyverse/lubridate#664) Fix system timezone memoization

Version 1.7.3
=============

### BUG FIXES

* [#643](tidyverse/lubridate#643), [#640](tidyverse/lubridate#640), [#645](tidyverse/lubridate#645) Fix faulty caching of system timezone.
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

3 participants