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

quarter() function bug #682

Closed
awfrankwils opened this issue Jun 12, 2018 · 7 comments
Closed

quarter() function bug #682

awfrankwils opened this issue Jun 12, 2018 · 7 comments

Comments

@awfrankwils
Copy link

@awfrankwils awfrankwils commented Jun 12, 2018

First, I would like to thank the authors @hadley @jiho @mmparker @zeehio @garrettgman for this package, and in particular the development of the quarter() function which does a great job of reducing the amount of code needed to generate FY and Quarter values. I would like to point out that there currently appears to be a bug in the way the quarter() function appends a fiscal year with_year.

Original example
x <- ymd(c("2012-03-26", "2012-05-04", "2012-09-23", "2012-12-31"))
quarter(x, with_year = TRUE, fiscal_start = 11)
[1] 2012.2 2012.3 2012.4 2013.1
This works fine with the december 2012 date reported as fiscal year 2013 Q1.

However if I move the fiscal start to April, we don't see similar treatment for fiscal year of the first date -- which should be 2011 Q4 based on this formatting.
quarter(x, with_year = TRUE, fiscal_start = 4)
[1] 2012.4 2012.1 2012.2 2012.3

@borgmaan
Copy link

@borgmaan borgmaan commented Jul 23, 2018

I believe there is still a bug in the function for fiscal_start of 7, 8, or 9. Here's what I'm seeing with the latest version installed from GitHub:

> x
[1] "2012-03-26" "2012-05-04" "2012-09-23" "2012-12-31"
> quarter(x, with_year = TRUE, fiscal_start = 7)
[1] 2012.3 2012.4 2012.1 2012.2
> quarter(x, with_year = TRUE, fiscal_start = 8)
[1] 2012.3 2012.4 2012.1 2012.2
> quarter(x, with_year = TRUE, fiscal_start = 9)
[1] 2012.3 2012.3 2012.1 2012.2

I'm running:

Session info ------------------------------------------------------------------------------------------------------------------------
 setting  value                       
 version  R version 3.4.4 (2018-03-15)
 system   x86_64, darwin15.6.0        
 ui       RStudio (1.1.453)           
 language (EN)                        
 collate  en_US.UTF-8                 
 tz       America/New_York            
 date     2018-07-23                  

With:

Packages ----------------------------------------------------------------------------------------------------------------------------
 package      * version  date       source                              
...
lubridate    * 1.7.4    2018-07-23 Github (tidyverse/lubridate@f7a7c27)
...

Loading

@vspinu
Copy link
Member

@vspinu vspinu commented Jul 23, 2018

Indeed. Looks like the logic of quarter is plain bogus. Will have to carefully dig into it.

Loading

@borgmaan
Copy link

@borgmaan borgmaan commented Jul 24, 2018

It's probably worth documenting the expected behavior of the function. FWIW, Wikipedia says:

The fiscal year is usually denoted by the year in which it ends, so United States federal government spending incurred on 14 November 2018 would belong to fiscal year 2019, operating on a fiscal calendar of October–September.

That seems to be the expected behavior of most of the tests and what is described in the bug report above. However, one of the tests from the commit closing this issue seems to violate that expectation.

I don't know what is right, but following that convention, I believe the test should be updated to:

x <- ymd(c("2012-03-26", "2012-05-04", "2012-09-23", "2012-03-01", "2012-12-31"))
expect_equal(quarter(x, with_year = TRUE, fiscal_start = 4),
             c(2012.4, 2013.1, 2013.2, 2012.4, 2013.3))

The following function passes all other tests (and the modified one above):

quarter <- function(x, with_year = FALSE, fiscal_start = 1) {
  fs <- fiscal_start - 1
  shifted <- seq(fs, 11 + fs) %% 12 + 1
  m <- month(x)
  quarters <- rep(1:4, each = 3)
  s <- match(m, shifted)
  q <- quarters[s]
  if (with_year) {
    uq <- quarters[m]
    inc_year <- (m >= fiscal_start) * (fiscal_start != 1)
    year(x) + inc_year + q/10
  }
  else q
}

Happy to submit a PR if this is the expected behavior and solves the issue. Here's the full diff for reference:

diff --git a/R/accessors-quarter.r b/R/accessors-quarter.r
index d299b64..c4bcf55 100644
--- a/R/accessors-quarter.r
+++ b/R/accessors-quarter.r
@@ -29,9 +29,8 @@ quarter <- function(x, with_year = FALSE, fiscal_start = 1) {
   q <- quarters[s]
   if (with_year) {
     uq <- quarters[m]
-    inc_year <- q == 1 & uq == 4
-    dec_year <- q == 4 & uq == 1
-    year(x) + inc_year - dec_year + q/10
+    inc_year <- (m >= fiscal_start) * (fiscal_start != 1)
+    year(x) + inc_year + q/10
   }
   else q
 }
diff --git a/tests/testthat/test-accessors.R b/tests/testthat/test-accessors.R
index 8685192..f7fe0fa 100644
--- a/tests/testthat/test-accessors.R
+++ b/tests/testthat/test-accessors.R
@@ -239,7 +239,7 @@ test_that("quarters accessor extracts correct quarter", {

   x <- ymd(c("2012-03-26", "2012-05-04", "2012-09-23", "2012-03-01", "2012-12-31"))
   expect_equal(quarter(x, with_year = TRUE, fiscal_start = 4),
-               c(2011.4, 2012.1, 2012.2, 2011.4, 2012.3))
+               c(2012.4, 2013.1, 2013.2, 2012.4, 2013.3))
   expect_equal(quarter(x, with_year = TRUE, fiscal_start = 11),
                c(2012.2, 2012.3, 2012.4, 2012.2, 2013.1))
 })

Loading

@rlh1994
Copy link

@rlh1994 rlh1994 commented Jan 28, 2019

@borgmaan Was there a resolution to this?

Loading

@mielniczuk
Copy link

@mielniczuk mielniczuk commented Jul 19, 2019

It appears the bug still exists for fiscal years starting in month 4 (or 3 or 2). I would expect that a FY starting April would show 2019-01-10 as FY 2018 Qtr 4 or 2018.4 result for the quarter() function when with_year=TRUE. Currently
quarter('2019-01-10', with_year = TRUE, fiscal_start = 4) produces 2019.4

Loading

@yhamade
Copy link

@yhamade yhamade commented Jul 23, 2019

While this doesn't conform to the "Fiscal Quarters" definition covered by @borgmaan, it does address the incorrect year for fiscal_start=4 outlined by @mielniczuk to produce an output of 2018.4 (Fiscal Years that START in April and end in Mach of the following year).

quarter <- function(x, with_year = FALSE, fiscal_start = 1) {
   fs <- fiscal_start - 1
   shifted <- seq(fs, 11 + fs) %% 12 + 1
   m <- month(x)
   quarters <- rep(1:4, each = 3)
   s <- match(m, shifted)
   q <- quarters[s]
   if (with_year) {
      uq <- quarters[m]
      ifelse(m <= fs, ((year(x)-1) + q/10), (year(x) + q/10))
   }
   else q
}

Loading

vspinu added a commit that referenced this issue Jun 1, 2020
@vspinu
Copy link
Member

@vspinu vspinu commented Jun 1, 2020

This issue slipped my radar as I forgot to re-open it back in 2018. It's fixed in the master and will be released to CRAN this week.

Loading

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
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants