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

Adding "quarter" option to floor/ceiling_date #303

Merged
merged 1 commit into from Feb 10, 2015

Conversation

Projects
None yet
2 participants
@jonboiser
Contributor

jonboiser commented Feb 5, 2015

In reference to Issue 239.

@vspinu

View changes

Show outdated Hide outdated R/round.r
switch(unit,
minute = minute(y) <- minute(y) + 1,
hour = hour(y) <- hour(y) + 1,
day = yday(y) <- yday(y) + 1,
week = week(y) <- week(y) + 1,
month = month(y) <- month(y) + 1,
year = year(y) <- year(y) + 1
year = year(y) <- year(y) + 1,
quarter = { month(y) <- month(y) + 3; y <- y - ddays(1) }

This comment has been minimized.

@vspinu

vspinu Feb 5, 2015

Member

Why do you need to subtract ddays(1)?

@vspinu

vspinu Feb 5, 2015

Member

Why do you need to subtract ddays(1)?

This comment has been minimized.

@jonboiser

jonboiser Feb 5, 2015

Contributor

The issue reporter wanted ceiling to return the last day of the quarter. This line adds three months to the first day of the quarter, then subtracts one (to handle months with 28/30/31 days), instead of adding two months, and having to figure out how long a month is to get the last day.

Is there a function in lubridate that gives the number of days like "number of days in february on a non-leap year = 28"?

@jonboiser

jonboiser Feb 5, 2015

Contributor

The issue reporter wanted ceiling to return the last day of the quarter. This line adds three months to the first day of the quarter, then subtracts one (to handle months with 28/30/31 days), instead of adding two months, and having to figure out how long a month is to get the last day.

Is there a function in lubridate that gives the number of days like "number of days in february on a non-leap year = 28"?

This comment has been minimized.

@vspinu

vspinu Feb 7, 2015

Member

This introduces an exception for quarter and we don't want that. OP can substract 1 day himself if so desired.

@vspinu

vspinu Feb 7, 2015

Member

This introduces an exception for quarter and we don't want that. OP can substract 1 day himself if so desired.

@jonboiser

View changes

Show outdated Hide outdated R/round.r
switch(unit,
minute = minute(y) <- minute(y) + 1,
hour = hour(y) <- hour(y) + 1,
day = yday(y) <- yday(y) + 1,
week = week(y) <- week(y) + 1,
month = month(y) <- month(y) + 1,
year = year(y) <- year(y) + 1
year = year(y) <- year(y) + 1,
quarter = month(y) <- month(y) + 3

This comment has been minimized.

@jonboiser

jonboiser Feb 9, 2015

Contributor

Removed that dday from this line. The behavior of ceiling is now to return the first day of the next quarter. The behavior of floor is to return the first day of the nearest quarter.

Actually, what would happen for dates in Q4 (e.g. november)? Would we maybe need to add the 3 months modulo 12, and then add some extra logic below to increment the year in this situation?

@jonboiser

jonboiser Feb 9, 2015

Contributor

Removed that dday from this line. The behavior of ceiling is now to return the first day of the next quarter. The behavior of floor is to return the first day of the nearest quarter.

Actually, what would happen for dates in Q4 (e.g. november)? Would we maybe need to add the 3 months modulo 12, and then add some extra logic below to increment the year in this situation?

This comment has been minimized.

@vspinu

vspinu Feb 9, 2015

Member

Actually, what would happen for dates in Q4 (e.g. november)?

What's the problem with those days?

@vspinu

vspinu Feb 9, 2015

Member

Actually, what would happen for dates in Q4 (e.g. november)?

What's the problem with those days?

This comment has been minimized.

@jonboiser

jonboiser Feb 9, 2015

Contributor

Oh wow, I just tried this out:

x <- ymd("2014-11-15")
month(x) <- month(x) + 3
x #=>  "2015-02-01 UTC"

That's pretty cool. I forget the OO terminology, but is it like month is an "active attribute" or something like that, such that if it is changed, the entire object is updated? I had assumed that it would return something like month(x) = 14 after the update.

Actually, this is interesting behavior too. So it truncates the mday to one as well?

@jonboiser

jonboiser Feb 9, 2015

Contributor

Oh wow, I just tried this out:

x <- ymd("2014-11-15")
month(x) <- month(x) + 3
x #=>  "2015-02-01 UTC"

That's pretty cool. I forget the OO terminology, but is it like month is an "active attribute" or something like that, such that if it is changed, the entire object is updated? I had assumed that it would return something like month(x) = 14 after the update.

Actually, this is interesting behavior too. So it truncates the mday to one as well?

This comment has been minimized.

@vspinu

vspinu Feb 9, 2015

Member

So it truncates the mday to one as well?

No. Something is wrong with your lubridate.

> x <- ymd("2014-11-15")
month(x) <- month(x) + 3
x
 > [1] "2015-02-15 UTC"
@vspinu

vspinu Feb 9, 2015

Member

So it truncates the mday to one as well?

No. Something is wrong with your lubridate.

> x <- ymd("2014-11-15")
month(x) <- month(x) + 3
x
 > [1] "2015-02-15 UTC"

This comment has been minimized.

@jonboiser

jonboiser Feb 9, 2015

Contributor

My mistake. I was looking at the output after changing x to 11/1.

@jonboiser

jonboiser Feb 9, 2015

Contributor

My mistake. I was looking at the output after changing x to 11/1.

@jonboiser

View changes

Show outdated Hide outdated R/round.r
@@ -39,7 +39,8 @@ floor_date <- function(x, unit = c("second","minute","hour","day", "week", "mont
day = update(x, hours = 0, minutes = 0, seconds = 0),
week = update(x, wdays = 1, hours = 0, minutes = 0, seconds = 0),
month = update(x, mdays = 1, hours = 0, minutes = 0, seconds = 0),
year = update(x, ydays = 1, hours = 0, minutes = 0, seconds = 0)
year = update(x, ydays = 1, hours = 0, minutes = 0, seconds = 0),
quarter = update(x, months = ((month(x) - 1) %/% 3)*3 + 1, mdays = 1, hours = 0, minutes = 0, seconds = 0)

This comment has been minimized.

@jonboiser

jonboiser Feb 9, 2015

Contributor

Truncated the smaller time units.

@jonboiser

jonboiser Feb 9, 2015

Contributor

Truncated the smaller time units.

@vspinu

View changes

Show outdated Hide outdated R/round.r
if (unit == "second") {
second(x) <- ceiling(second(x))
return(x)
}
if(unit != "quarter") {

This comment has been minimized.

@vspinu

vspinu Feb 9, 2015

Member

You still have this special treatment of "quarter" here.

@vspinu

vspinu Feb 9, 2015

Member

You still have this special treatment of "quarter" here.

@jonboiser

View changes

Show outdated Hide outdated R/round.r
@@ -159,10 +161,10 @@ parse_unit_spec <- function(unitspec) {
mult <- as.numeric(parts[[1]])
unit <- parts[[2]]
}

This comment has been minimized.

@jonboiser

jonboiser Feb 9, 2015

Contributor

I was looking at one your PRs and Hadley mentioned cleaning up the diffs to not show stuff like this. How do you remove diffs like this which appear from adding/removing newline characters?

@jonboiser

jonboiser Feb 9, 2015

Contributor

I was looking at one your PRs and Hadley mentioned cleaning up the diffs to not show stuff like this. How do you remove diffs like this which appear from adding/removing newline characters?

This comment has been minimized.

@vspinu

vspinu Feb 9, 2015

Member

Seems not to be easy. http://stackoverflow.com/questions/3515597/git-add-only-non-whitespace-changes

Just don't introduce those in the first place. How did you introduce so many whitespace changes?

I am usually unstaging manually those with magit in emacs. You have to figure out how to do that in your front end.

@vspinu

vspinu Feb 9, 2015

Member

Seems not to be easy. http://stackoverflow.com/questions/3515597/git-add-only-non-whitespace-changes

Just don't introduce those in the first place. How did you introduce so many whitespace changes?

I am usually unstaging manually those with magit in emacs. You have to figure out how to do that in your front end.

This comment has been minimized.

@jonboiser

jonboiser Feb 9, 2015

Contributor

They first showed up when I added a new line to the switch statement in ceiling_date and shifted the contents below it. I've never really thought of these whitespace modifications before, but in RStudio, you can discard the these kinds of chunks so they don't appear in the commit.

Is this PR almost finished? Do the tests and docs need to be modified to reflect this addition?

@jonboiser

jonboiser Feb 9, 2015

Contributor

They first showed up when I added a new line to the switch statement in ceiling_date and shifted the contents below it. I've never really thought of these whitespace modifications before, but in RStudio, you can discard the these kinds of chunks so they don't appear in the commit.

Is this PR almost finished? Do the tests and docs need to be modified to reflect this addition?

This comment has been minimized.

@vspinu

vspinu Feb 9, 2015

Member

Is this PR almost finished? Do the tests and docs need to be modified to reflect this

Unit test fail.

Github doesn't show your indentation right. It's completely messed up. Are you using TABs to indent your code? Please use simple spaces. R studio must have an option for that.

Also please squash all the commits into one. You can add one test for some corner case.

Thanks.

@vspinu

vspinu Feb 9, 2015

Member

Is this PR almost finished? Do the tests and docs need to be modified to reflect this

Unit test fail.

Github doesn't show your indentation right. It's completely messed up. Are you using TABs to indent your code? Please use simple spaces. R studio must have an option for that.

Also please squash all the commits into one. You can add one test for some corner case.

Thanks.

This comment has been minimized.

@vspinu

vspinu Feb 9, 2015

Member

Unit test fail.

Ok. It's the doc warning. We can leave with this. If it's not difficult, could you please re-roxygenize with the dev version of roxygen2 or adjust the Rd manually.

@vspinu

vspinu Feb 9, 2015

Member

Unit test fail.

Ok. It's the doc warning. We can leave with this. If it's not difficult, could you please re-roxygenize with the dev version of roxygen2 or adjust the Rd manually.

@jonboiser

This comment has been minimized.

Show comment
Hide comment
@jonboiser

jonboiser Feb 10, 2015

Contributor

Took awhile, but got the PR all cleaned up with a minimal diff.

Contributor

jonboiser commented Feb 10, 2015

Took awhile, but got the PR all cleaned up with a minimal diff.

vspinu added a commit that referenced this pull request Feb 10, 2015

Merge pull request #303 from jonboiser/master
Adding "quarter" option to floor/ceiling_date

@vspinu vspinu merged commit 5ecc319 into tidyverse:master Feb 10, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@vspinu

This comment has been minimized.

Show comment
Hide comment
@vspinu

vspinu Feb 10, 2015

Member

👍 Thanks.

Member

vspinu commented Feb 10, 2015

👍 Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment