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

Allow dt argument of tzDiff to be a date (possibly vector) #24

Merged
merged 4 commits into from Nov 28, 2017

Conversation

Projects
None yet
2 participants
@anderic1
Contributor

anderic1 commented Nov 24, 2017

Not sure this is the correct procedure as I am new to github. Anyway this PR allows the dt argument of tzDiff to be (1) of Date or POSIXct class, and (2) possibly a vector

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Nov 24, 2017

Owner

Looks like a rather clean patch / pull request. The main problem I have is logical. Date objects are known not have a TZ attribute so why do you want to shift something that doesn't exist? Or do you use Date as a proxy for midnight?

(And it is this type of question why I sometimes [ie other repos] spell out "issue ticket discussion and consensus before pull request" as the preferred way. But we'll get this squared.)

Owner

eddelbuettel commented Nov 24, 2017

Looks like a rather clean patch / pull request. The main problem I have is logical. Date objects are known not have a TZ attribute so why do you want to shift something that doesn't exist? Or do you use Date as a proxy for midnight?

(And it is this type of question why I sometimes [ie other repos] spell out "issue ticket discussion and consensus before pull request" as the preferred way. But we'll get this squared.)

@anderic1

This comment has been minimized.

Show comment
Hide comment
@anderic1

anderic1 Nov 24, 2017

Contributor

Yes, Date as a proxy for midnight UTC as this is what R does when converting a Date to POSIXct. I hesitated to do the conversion at midnight of the fromtz time zone, but figured the added complexity was not warranted, in addition to that this is not what R does.
From a practical point of view I still think it is useful to allow dt to be a Date, but I agree with you that strictly speaking it is not correct. If you wish I could remove that possibility and make it error out for a Date, thus only keeping the vectorization part.

Contributor

anderic1 commented Nov 24, 2017

Yes, Date as a proxy for midnight UTC as this is what R does when converting a Date to POSIXct. I hesitated to do the conversion at midnight of the fromtz time zone, but figured the added complexity was not warranted, in addition to that this is not what R does.
From a practical point of view I still think it is useful to allow dt to be a Date, but I agree with you that strictly speaking it is not correct. If you wish I could remove that possibility and make it error out for a Date, thus only keeping the vectorization part.

@anderic1 anderic1 referenced this pull request Nov 24, 2017

Closed

Unexpected result tzDiff #23

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Nov 27, 2017

Owner

This looks good. And sorry to spring this on you now, but could you look at what Rcpp::newDatetimeVector which is now Rcpp::DatetimeVector by default (in Rcpp 0.12.14) could do for your? The use of NumericVector was kludge I used when we only had what is now oldDatetimeVector.

We could push that off to a 2nd PR. No rush.

Owner

eddelbuettel commented Nov 27, 2017

This looks good. And sorry to spring this on you now, but could you look at what Rcpp::newDatetimeVector which is now Rcpp::DatetimeVector by default (in Rcpp 0.12.14) could do for your? The use of NumericVector was kludge I used when we only had what is now oldDatetimeVector.

We could push that off to a 2nd PR. No rush.

@anderic1

This comment has been minimized.

Show comment
Hide comment
@anderic1

anderic1 Nov 27, 2017

Contributor

Are you referring to the input parameter being a NumericVector?
I actually did some experiments with this:

#include <Rcpp.h>

using namespace Rcpp;

// [[Rcpp::export]]
String fnum(NumericVector v) 
{
    if(v.inherits("Date"))
        return "Date";
    
    if(v.inherits("POSIXct"))
        return "POSIXct";
    
    return "";
}


// [[Rcpp::export]]
String fdatetime(DatetimeVector v) 
{
    if(v.inherits("Date"))
        return "Date";
    
    if(v.inherits("POSIXct"))
        return "POSIXct";
    
    return "";
}


/***R
lvals = list(num=3.14, Date=Sys.Date(), Datetime=Sys.time())
sapply(lvals, function(x) c(fnum=fnum(x), fdatetime=fdatetime(x)))
#           num       Date      Datetime 
# fnum      ""        "Date"    "POSIXct"
# fdatetime "POSIXct" "POSIXct" "POSIXct"
*/

Letting the argument be a Datetime coerces the input vector to a POSIXct-object (in the same vein as integer -> numeric). Whereas letting it be a NumericVector keeps the class attribute which is useful in this PR.
Unless of course you are referring to something else.

Contributor

anderic1 commented Nov 27, 2017

Are you referring to the input parameter being a NumericVector?
I actually did some experiments with this:

#include <Rcpp.h>

using namespace Rcpp;

// [[Rcpp::export]]
String fnum(NumericVector v) 
{
    if(v.inherits("Date"))
        return "Date";
    
    if(v.inherits("POSIXct"))
        return "POSIXct";
    
    return "";
}


// [[Rcpp::export]]
String fdatetime(DatetimeVector v) 
{
    if(v.inherits("Date"))
        return "Date";
    
    if(v.inherits("POSIXct"))
        return "POSIXct";
    
    return "";
}


/***R
lvals = list(num=3.14, Date=Sys.Date(), Datetime=Sys.time())
sapply(lvals, function(x) c(fnum=fnum(x), fdatetime=fdatetime(x)))
#           num       Date      Datetime 
# fnum      ""        "Date"    "POSIXct"
# fdatetime "POSIXct" "POSIXct" "POSIXct"
*/

Letting the argument be a Datetime coerces the input vector to a POSIXct-object (in the same vein as integer -> numeric). Whereas letting it be a NumericVector keeps the class attribute which is useful in this PR.
Unless of course you are referring to something else.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Nov 27, 2017

Owner

Nice example. Now, and I may be dense (or preoccupied while at work) but why would we want a different argument? I guess because here we talk about tzDiff so we want a number? In which case you are right and my point about changing this input type was wrong :)

Owner

eddelbuettel commented Nov 27, 2017

Nice example. Now, and I may be dense (or preoccupied while at work) but why would we want a different argument? I guess because here we talk about tzDiff so we want a number? In which case you are right and my point about changing this input type was wrong :)

@anderic1

This comment has been minimized.

Show comment
Hide comment
@anderic1

anderic1 Nov 28, 2017

Contributor

Sorry, I was not clear enough, hopefully this is clearer :)

If the signature was double tzDiff(..., DatetimeVector dt, ...) then in a call like tzDiff(dt=1, ...), the dt would be interpreted as one second after midnight 1970-01-01 (as per the coercion to a DatetimeVector).

On the other hand letting the signature be double tzDiff(..., NumericVector dt, ...) that same call would yield an error (Rcpp::stop("Unhandled date class")), as would a call like tzDiff(dt=Sys.Date(), ...). This is because the class attribute is kept intact. This basically allows us to "dispatch" on the class of dt and only return a result if the input dt was a true POSIXct object. There might be a more natural way to achieve the same thing of which I am not aware...

Contributor

anderic1 commented Nov 28, 2017

Sorry, I was not clear enough, hopefully this is clearer :)

If the signature was double tzDiff(..., DatetimeVector dt, ...) then in a call like tzDiff(dt=1, ...), the dt would be interpreted as one second after midnight 1970-01-01 (as per the coercion to a DatetimeVector).

On the other hand letting the signature be double tzDiff(..., NumericVector dt, ...) that same call would yield an error (Rcpp::stop("Unhandled date class")), as would a call like tzDiff(dt=Sys.Date(), ...). This is because the class attribute is kept intact. This basically allows us to "dispatch" on the class of dt and only return a result if the input dt was a true POSIXct object. There might be a more natural way to achieve the same thing of which I am not aware...

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Nov 28, 2017

Owner

Agreed! Patch is good, will merge. You didn't update the help page part either, but that is ok. I'll add ChangeLog and NEWS as usual.

It's good to have that one vectorized.

Owner

eddelbuettel commented Nov 28, 2017

Agreed! Patch is good, will merge. You didn't update the help page part either, but that is ok. I'll add ChangeLog and NEWS as usual.

It's good to have that one vectorized.

@eddelbuettel eddelbuettel merged commit de9ee60 into eddelbuettel:master Nov 28, 2017

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Nov 29, 2017

Owner

What name would you like to be listed with in the changelog?

Owner

eddelbuettel commented Nov 29, 2017

What name would you like to be listed with in the changelog?

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