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

Inconsistent results dividing an interval by a period #926

Closed
petehobo opened this issue Oct 22, 2020 · 6 comments
Closed

Inconsistent results dividing an interval by a period #926

petehobo opened this issue Oct 22, 2020 · 6 comments
Labels
bug an unexpected problem or unintended behavior

Comments

@petehobo
Copy link

I'm using current version of lubridate (1.7.9) from CRAN.

While investigating some code left by a predecessor, I found the following inconsistency:

library(lubridate)
interval(ymd('2019-01-29'), ymd('2019-01-30'))/months(1)  # 0.06451613
interval(ymd('2020-01-29'), ymd('2020-01-30'))/months(1)  # 0.03225806

The 2019 figure seems wrong, equating to 2 (/31) days, when it should be 1.

I have a workaround using time_length(), which returns consistent results

time_length(interval(ymd('2019-01-29'), ymd('2019-01-30')), 'month')  # 0.03225806
time_length(interval(ymd('2020-01-29'), ymd('2020-01-30')), 'month')  # 0.03225806

Perhaps the approach taken by my predecessor is an anti-pattern, but I figured it was worth raising, as it does seem like a bug.

@petehobo
Copy link
Author

petehobo commented Oct 26, 2020

A little more digging shows the difference seems to come from the adjust_estimate() function. In both these cases, estimate is 0.03285421, but adjust_estimate() returns different values:

library(lubridate)

p <- months(1)

i <- interval(ymd('2019-01-29'), ymd('2019-01-30'))
estimate <- i/as.duration(p) # 0.03285421
lubridate:::adjust_estimate(estimate, i, p)

[1] 0.06451613

i <- interval(ymd('2020-01-29'), ymd('2020-01-30'))
estimate <- i/as.duration(p) # 0.03285421
lubridate:::adjust_estimate(estimate, i, p)

[1] 0.03225806

I assume it comes down to leap year/non-leap year differences, but I haven't investigated further.

@vspinu
Copy link
Member

vspinu commented Oct 26, 2020

Thank @petehobo. I am on it. The problem of division by periods is not well defined, but at least in obvious cases like your exampe it should give meaningful results.

@petehobo
Copy link
Author

Thanks @vspinu . Yeah, I figured it was messy - that's why I stopped digging where I did...

Thanks for looking into it - lubridate is such a useful library

@oude-gao
Copy link

oude-gao commented Feb 1, 2021

I'm experiencing the same issue -- is there a plan to apply a fix? I have been using the division for a long time and just realized the inconsistency today. Am imagining a lot of people are doing the same thing, so if there is a quick fix it would be greatly appreciated! Thanks!!

@vspinu vspinu added the bug an unexpected problem or unintended behavior label Feb 24, 2021
@vspinu vspinu closed this as completed in ae95ab5 Feb 24, 2021
@vspinu
Copy link
Member

vspinu commented Feb 24, 2021

Thanks guys for reporting and sorry for the delay. Should be fixed now.

@petehobo
Copy link
Author

Excellent - thanks @vspinu! Looks good

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
bug an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

3 participants