Counter intuitive result when using anydate, is it a config problem I have ? #36

Closed
statquant opened this Issue Dec 12, 2016 · 18 comments

Projects

None yet

2 participants

@statquant

I am trying to use anytime those days and I see results that seems counter intuitive to me.
I have the following:

anytime:::getTZ()
[1] "Europe/London"
anytime('2016-05-12 15:00:00')
[1] "2016-05-12 14:00:00 BST" # which I would have expected to be "2016-05-12 15:00:00 BST"
anytime('2016-12-12 15:00:00')
[1] "2016-12-12 14:00:00 GMT" # which I would have expected to be "2016-05-12 15:00:00 GMT"

Reading the vignette I understand that from
function (x, tz = getTZ(), asUTC = FALSE) that tz is NOT the timezone anytime uses to parse the data, which defaults to local (or UTC if asUTC is TRUE) but for "display". This makes sense as when you read data (from a string) you probably assume that the data is in your timezone.

When I use anydate I then get

anydate('2016-12-12')         
[1] "2016-12-11"
anydate(as.Date('2016-12-12')) # as as.Date('2016-12-12') is already a date 
[1] "2016-12-12"

Is this the expected behaviour ?
I am not sure what I miss here.

@statquant statquant changed the title from Is the default of tz for anyDate to Counter intuitive result when using anydate, is it a config problem I have ? Dec 12, 2016
@eddelbuettel
Owner

That's a bug, and I can replicate it here when I use "TZ=Europe/London". Oddly, it does not happen for either my TZ or UTC.

If you have a moment, can you chase it down? As you can see, the anydate() function does very little. We probably "just" have to add another tz=... somewhere.

@eddelbuettel
Owner

It behaves better when you use utcdate(). I don't quite understand why it is otherwise off by an hour.

@statquant

I will try to track it down this weekend. Thanks for answering

@eddelbuettel
Owner

Appreciate the help and second set of eyes.

Some comtext: There is a (really long) issue thread #5 -- incidentally with the same problem right in the title -- happening for one locale in Australia (and I got a lot of testing help from @jason-turner2). This now makes it two. I am starting to suspect that it may be a Boost Date_time issue. I tried some variants around TZ=Europe/London yesterday and got really weird results with it either being an hour ahead or behind. Converting via as.numeric() and looking at a (web-based) epoch converter helps a little.

A simple test may just be to use strptime from the system, or from R, and see what happens. Maybe I need to refactor that part of the package. I should at least do some more testing on this.

I'll probably release 0.1.2 in the meantime as it fixes one other corner case bug.

@eddelbuettel
Owner

I am still not entirely sure where we loose that hour, but I think the best way about is to make anydate() a proper accessor and to convert from Boost Posix Time to Boost Gregorian::Date at the C++ side and then just export it. That does mean rejigging some code so it won't be immediate, but I hope to get to it "at some point".

@statquant

Hello, I tried to create a pull request but could not do it.
The following change works for me, I might be doing something stupid that breaks everything else but I am not sure why the epoch had to be in Local.
I am new to working on r packages, I ran your tests and did not see anything that I would have broken.
Something else: using Rstudio and rebuilding the sources I randomely get stupid results like :

> anytime("2016-07-11")
[1] "1400-01-01 09:12:08 LMT"

and I need to close RStudio, rebuild to get it working again (I cannot reproduce)...

// given a ptime object, return (fractional) seconds since epoch
// account for localtime, and also account for dst
double ptToDouble(const bt::ptime & pt) {

    const bt::ptime timet_start(boost::gregorian::date(1970,1,1));
    bt::time_duration tdiff = pt - timet_start;

    // hack-ish: go back to struct tm to use its tm_isdst field
    time_t secsSinceEpoch = tdiff.total_seconds();
    struct tm* localAsTm = localtime(&secsSinceEpoch);
    //Rcpp::Rcout << "Adj is " << localAsTm->tm_isdst << std::endl;

    // Define BOOST_DATE_TIME_POSIX_TIME_STD_CONFIG to use nanoseconds
    // (and then use diff.total_nanoseconds()/1.0e9;  instead)
    //
    // note dst correction here -- needed as UTC offset is correct but does not
    // contain the additional DST adjustment
    double totsec = tdiff.total_microseconds()/1.0e6, dstadj = 0;
#if defined(_WIN32)
    if (totsec > 0) {           // on Windows, for dates before 1970-01-01: segfault
        dstadj = localAsTm->tm_isdst*60*60;
    }
#else
    dstadj = localAsTm->tm_isdst*60*60;
#endif
    return totsec - dstadj;
}

Stuff from Autralia/Sydney:

Sys.setenv(TZ = "Australia/Sydney")
library(anytime)
anydate(20150101)
[1] "2015-01-01"

Current issue Europe/London:

anydate(20150101)
[1] "2015-01-01"
anydate('2016-12-12')
[1] "2016-12-12"
anytime('2016-12-12 15:00:00')
[1] "2016-12-12 15:00:00 GMT"
anytime('2016-05-12 15:00:00')
[1] "2016-05-12 15:00:00 BST"
@eddelbuettel
Owner
eddelbuettel commented Dec 18, 2016 edited

Let's take this one step at a time:

  • if in doubt do NOT use RStudio with anytime. Maybe build, but do not run
  • it is known to crash on some operations
  • see an issue ticket here: #25
  • see how two functions abort (at the R level) when we see we are in RStudio, that was #27
  • likely due to us using Boost, and .... RStudio being built against a different (ancient version) of Boost
  • they know about this, I know about this, there is no immediate fix other than 'do not do this'

Updated:

  • I can still run 'check' fine too in RStudio
@eddelbuettel
Owner

That out of the way, you can still try to build in RStudio if you don't know how to build otherwise.

Just try to run more tests on the command-line, maybe via RScript.

Now: can you detail what you changed where? Did you commit something somewhere?

@eddelbuettel
Owner

Ok, I created a branch with the (shorter) version of ptToDouble() above. You may be on to something as I just noticed this in R itself yesterday:

R> as.POSIXct.numeric
function (x, tz = "", origin, ...) 
{
    if (missing(origin)) 
        stop("'origin' must be supplied")
    .POSIXct(as.POSIXct(origin, tz = "GMT", ...) + x, tz)
}
<bytecode: 0x5868e40>
<environment: namespace:base>
R> 

Here too the 'epoch timepoint' is computed with tz="GMT". I had the local adjustment code in all my versions starting from some Boost Date_Time examples ... including in the RcppBDT package.

@statquant
statquant commented Dec 18, 2016 edited

Rstudio: I usually not use it at all, that yet another reason, I had a Boost version mismatch bug on some unrelated project a week ago so I get it.

What I changed: Only the 2 first lines of ptToDouble in anytime.cpp when you were setting the tz of the 1970-01-01 epoch to local (using code you got from the boost help ). My view was that the epoch should be in UTC and not in local, for the offset to be accurate (cf how R does it).

Did I commit: No I did not, I am not sure how to, I could clone, create a copy-repo and commit, but I thought the idiomatic way was to create a pull request.

@eddelbuettel
Owner
eddelbuettel commented Dec 18, 2016 edited

And the trouble is if I do what you suggest, it will only work in Greenwich, UK:

edd@max:~/git/anytime(feature/improved_pttodouble)$ date
Sun Dec 18 10:16:43 CST 2016
edd@max:~/git/anytime(feature/improved_pttodouble)$ Rscript -e 'anytime::anytime("2016-12-18 10:16:43")'
[1] "2016-12-18 04:16:43 CST"
edd@max:~/git/anytime(feature/improved_pttodouble)$ 

That's just plain wrong by six hours.

@eddelbuettel
Owner
eddelbuettel commented Dec 18, 2016 edited

I had a Boost version mismatch bug on some unrelated project a week ago so I get it.

Super-annoying as hard to fix. In all these years with Rcpp and BH this is the first one from those projects. There must be a Date_Time object instantiation somewhere on each side.

What I changed:

Got that now, see above.

Did I commit: No I am not sure how to, I could clone, create a copy-repo and commit, but I thought the idiomatic way was to create a pull request.

That is how you create a pull request. You clone (or fork), commit your change and the pull request is based the difference between your repo (or branch) and the repo you send the PR to, ie my master.

Useful to learn that, and this repo is admirably small that you may as well learn.

@statquant

Ok, I feel this is along "Boost sets the local timezone on construction, R expects UTC as epoch, I want another timezone", please bear with me, I am on a blocked eurostar with limited wifi, I'll get back to it.

@eddelbuettel
Owner

What you suggest is something we may already have in the package. Did you ever look at utctime() and utcdate() ?

@eddelbuettel
Owner

I have it fixed now -- by converting to Date internally in the C++ code:

edd@max:~/git/anytime(feature/improved_pttodouble)$ TZ="Europe/London" r -lanytime -p -e'at <- anytime:::anytime_cpp("2016-12-18 00:00", asUTC=FALSE, asDate=TRUE)'
[1] "2016-12-18"
edd@max:~/git/anytime(feature/improved_pttodouble)$ TZ="Europe/London" r -lanytime -p -e'at <- anydate("2016-12-18 00:00")'
[1] "2016-12-17"
edd@max:~/git/anytime(feature/improved_pttodouble)$ 

Here the first one correctly returns Dec 18 in your case of a TZ for London. It uses the new (internal) argument asDate.

For comparison anydate() (which has simply not yet been updated) shows the wrong earlier behaviour.

@eddelbuettel
Owner

This is now fixed in the master branch.

@statquant

Running the lattest anytime I still see the following behaviour:

library(anytime)
anytime:::getTZ()
[1] "Europe/London"
anytime('2016-05-12 15:00:00')
[1] "2016-05-12 14:00:00 BST" ## which I would have expected to be "2016-05-12 15:00:00 BST"
anytime('2016-12-12 15:00:00')
[1] "2016-12-12 14:00:00 GMT" ## which I would have expected to be "2016-05-12 15:00:00 GMT"

Do I misunderstand the expected behaviour ?

@statquant

Ah it looks like it is related to Europe/London TZ Goes back an Hour #51

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