bugGNU Octave - Bugs: bug #36954, datenum / datevec convert...

 
 

bug #36954: datenum / datevec convert date-time string incorrectly during hour lost to DST

Submitted by:  None
Submitted on:  Thu 26 Jul 2012 02:42:58 AM UTC  
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  Mike Miller <mtmiller>
Originator Name:  Originator Email:  -email is unavailable-
Open/Closed:  Closed Release:  dev
Operating System:  Any

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Fri 01 Sep 2017 10:24:04 PM UTC, comment #37: 

@Mike
I don't think it is quite your fault that this took so long.

Philip Nienhuis <philipnienhuis>
Project Member
Fri 01 Sep 2017 09:59:20 PM UTC, comment #36: 

Good, let's call this fixed for now, and you know where to come if you still hit problems in the future :)

Sorry this took so long to fix.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Fri 01 Sep 2017 09:50:02 PM UTC, comment #35: 

The test that I had in my patch in comment #12 seems to pass for my Windows 7 system. So I think this is good work, Mike, thanks.

In the next weeks I'll probably will do some more intensive testing once I'll pick up again my tidal analyses research (= where I first hit this bug).

Philip Nienhuis <philipnienhuis>
Project Member
Tue 29 Aug 2017 06:07:43 PM UTC, comment #34: 

I pushed this fix on the default branch

https://hg.savannah.gnu.org/hgweb/octave/rev/df726a12c757

Please build and test when you can on Windows. As we showed, the unit test won't work reliably on Windows, but you can test date strings that you know would be problematic in your local time zone.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Sat 26 Aug 2017 08:48:18 PM UTC, comment #33: 

In Windows there is a tzutil command that returns the time zone.
Output for my (US-English) system in CET time zone from Octave:

[~, tz] = system ("tzutil /g");
tz
W. European Standard Time

Could this be of help?

Philip Nienhuis <philipnienhuis>
Project Member
Sat 26 Aug 2017 08:42:08 PM UTC, comment #32: 

It's not that bad, it just means the test case will only work on Unixy systems.

The good news is the actual fix I wrote will work, because your output did not show the bug occurring when TZ is set to "UTC0". So the system does support the TZ variable, it probably just expects its own formats.

I'll add a "if (isunix ())" to the test and this can be considered fixed. If someone can extend the test later to use some kind of TZ variable that Windows supports, then we can look at extending it to work there also.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Sat 26 Aug 2017 08:31:43 PM UTC, comment #31: 

I'm afraid it isn't what you and I hoped for, (This is on Win7 prof. 64bit)
AFAIK Windows doesn't keep its TZ setting in environments strings, it's rather in the registry.
Using the OF-windows pkg that can be accessed invoking simple calls. But for core Octave that doesn't help.

Maybe this can help?
https://stackoverflow.com/questions/12765650/mingw-msys-msvcrt-and-the-tz-environment-variable

BTW I read there that MSYS2 is better in somw wys as far as TZ goes - ISTR that Tatsuro a while ago also suggested to switch to MSYS2 for mxe builds.

Philip Nienhuis <philipnienhuis>
Project Member
Sat 26 Aug 2017 08:16:35 PM UTC, comment #30: 

Here we go:

bug #36954 occurs on 2017-03-26 02:15:00 with TZ=EST+5EDT,M3.2.0/2,M11.1.0/2
bug #36954 occurs on 2017-03-26 02:15:00 with TZ=CET-1CEST,M3.5.0/2,M10.5.0/2
bug #36954 occurs on 2017-03-26 02:15:00 with TZ=CLT+4CLST,M8.2.0/0,M5.2.0/0
bug #36954 occurs on 2017-03-26 02:15:00 with TZ=LHST-10:30LHDT-11,M10.1.0/2,M4.1.0/2
bug #36954 occurs on 2017-03-26 02:15:00 with TZ=:America/Caracas

Philip Nienhuis <philipnienhuis>
Project Member
Sat 26 Aug 2017 03:44:23 PM UTC, comment #29: 

Sure, of course I'll do but then I first need to cross-build an unpatched ("pure" default) Octave then (my builds usually contain various other patches). Will do tonight

Philip Nienhuis <philipnienhuis>
Project Member
Fri 25 Aug 2017 08:14:42 PM UTC, comment #28: 

Oops, there's a small typo in the output, but the test is correct, the third string is actually "2017-08-13 00:15:00". That is what is actually being passed to datevec, only the output is wrong when I copy-pasted it.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Fri 25 Aug 2017 08:11:58 PM UTC, comment #27: 

Philip - can you please run the attached script tztest.m on Windows with an unpatched Octave? Either dev or 4.2 should be fine. I want to see what output you get, I want to know if setting the TZ environment variable does anything on Windows, and if it accepts the same time zone specification formats as POSIX.

Here's what you should see

>> tztest
bug #36954 occurs on 2017-03-12 02:15:00 with TZ=EST+5EDT,M3.2.0/2,M11.1.0/2
bug #36954 occurs on 2017-03-26 02:15:00 with TZ=CET-1CEST,M3.5.0/2,M10.5.0/2
bug #36954 occurs on 2017-03-13 00:15:00 with TZ=CLT+4CLST,M8.2.0/0,M5.2.0/0
bug #36954 occurs on 2017-10-01 02:15:00 with TZ=LHST-10:30LHDT-11,M10.1.0/2,M4.1.0/2
bug #36954 occurs on 2016-05-01 02:40:00 with TZ=:America/Caracas

This is basically the test that I am going to add for this fix, but I want to know if it will work on Windows or if I should flag it to run on Unixy systems only, which would be a shame.

(file #41659)

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Wed 23 Aug 2017 06:06:21 PM UTC, comment #26: 

I've updated the patch a bit, just added a comment and moved things around a little.

I'll try to work up some test cases for a few different time zones.

I think the best approach for the tests would be to iterate over a few different time zone specifications, and make sure that a set of test cases all work, each test sample being something that would fail in a particular time zone.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Wed 23 Aug 2017 05:53:33 PM UTC, comment #25: 

There is a test in my patch that should at least be valid for Central European Time

@Mike:
It is obvious that your way is much more robust. But then I never claimed that mine was good.
I'll try yours later this week (got an exam next Friday)

Philip Nienhuis <philipnienhuis>
Project Member
Wed 23 Aug 2017 05:29:42 PM UTC, comment #24: 

Please try the attached patch with a test case that works for you in your time zone. Also please make sure that time functions work normally and return the correct current local time after testing this in the same instance of Octave.

This change sets TZ=UTC0 for the block where strptime is used to convert a string into a struct tm. This ensures that no local time zone information is factored in to the conversion at all. It then tries to restore TZ by either unsetting it or setting it back if it had a value.

The only missing part here is that I am not sure if there is a way to tell whether an environment variable is missing or is set to an empty string in the m-file language, which is an important distinction here.

(file #41626)

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Wed 23 Aug 2017 07:52:40 AM UTC, comment #23: 

Sorry, one last post. To be clear, these examples were done without the patch applied. With the patch applied, I believe it would have overcompensated in the other direction, by subtracting a full hour.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Wed 23 Aug 2017 07:46:08 AM UTC, comment #22: 

Here is one case that breaks the approach in this patch:

>> getenv TZ
ans = America/Caracas
>> datevec ('2016-05-01 02:25:00')
ans =

   2016      5      1      2     25      0

>> datevec ('2016-05-01 02:30:00')
ans =

   2016      5      1      3      0      0

>> datevec ('2016-05-01 02:40:00')
ans =

   2016      5      1      3     10      0

Here is another case

>> getenv TZ
ans = Australia/LHI
>> datevec ('2017-10-01 01:50:00')
ans =

   2017     10      1      1     50      0

>> datevec ('2017-10-01 02:00:00')
ans =

   2017     10      1      2     30      0

>> datevec ('2017-10-01 02:10:00')
ans =

   2017     10      1      2     40      0

>> datevec ('2017-10-01 02:20:00')
ans =

   2017     10      1      2     50      0

>> datevec ('2017-10-01 02:30:00')
ans =

   2017     10      1      2     30      0

Source: https://www.timeanddate.com/time/time-zones-interesting.html

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Wed 23 Aug 2017 07:40:03 AM UTC, comment #21: 

Thanks,

I have changing the status to "In Progress".

Avinoam Kalma <avinoam>
Project Member
Wed 23 Aug 2017 07:35:13 AM UTC, comment #20: 

Yes, the patch also worked for me with minimal testing, and with no noticable performance impact. It makes me uncomfortable to fix this problem by converting the time structure back into a string and running regular expressions and string matching on it, there has got to be a better way... I just don't know what that is.

Now that I've done a bit of research, I also think this approach is fragile because it assumes all daylight savings shifts are exactly one hour, which I have just learned is not always the case.

The test case in this patch is definitely not useful, because it's entirely dependent on the user's local time zone setting.

I think this still needs some work and testing, and maybe a different approach. I'll see if I can come up with some cases that can't be solved this way.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Mon 21 Aug 2017 12:58:47 PM UTC, comment #19: 

... morph it ....

Philip Nienhuis <philipnienhuis>
Project Member
Mon 21 Aug 2017 12:58:26 PM UTC, comment #18: 

The status is that it "works for me", in my timezone (CEST).

Someone more knowledgeable in DST etc. will have to morph into something that has a chance to be included in core Octave. I don't have that knowledge and I lack time/urgency.

As to Mike -IMO legitimate- performance concerns, just above the place where my patch would apply there are already three consecutive unconditionally executed strrep statements - 'nuf said.

Philip Nienhuis <philipnienhuis>
Project Member
Mon 21 Aug 2017 10:33:53 AM UTC, comment #17: 

What is the status of the patch from comment #12?

Avinoam Kalma <avinoam>
Project Member
Sat 01 Jul 2017 12:49:03 PM UTC, comment #16: 

Here's some timings (on Win7 64-bit, octave-4.3.0+ as of late May)

I used the following script that I ran twice w. & w/o patch:

t_tot = t_max = 0;
t_min = Inf;
dats = [datenum(2009, 1, 1, 0, 1, 0) : (1/24/6) : datenum(2009, 12, 31, 23, 51, 0)];
for ii=1:100
  tic; datv = datevec (dats); tt = toc;
  t_tot += tt;
  t_min = min (t_min, tt);
  t_max = max (t_max, tt);
endfor
t_avg = t_tot / 100;
t_tot
t_min
t_max
t_avg

Without my patch:
+verbatim-
Run 1:

>> t_tot

t_tot =  1.1781

>> t_min

t_min =  0.011000

>> t_max

t_max =  0.029002

>> t_avg

t_avg =  0.011781

Run 2:

>> t_tot

t_tot =  1.1891

>> t_min

t_min =  0.011000

>> t_max

t_max =  0.032002

>> t_avg

t_avg =  0.011891
-verbatim-

... and with the patch:

Run 1:

>> t_tot
t_tot =  1.1771
>> t_min
t_min =  0.011000
>> t_max
t_max =  0.028002
>> t_avg
t_avg =  0.011771

Run 2:

>> t_avg = t_tot / 100;
>> t_tot
t_tot =  1.1981
>> t_min
t_min =  0.011000
>> t_max
t_max =  0.021002
>> t_avg
t_avg =  0.011981

I'd say there's no discernable slowdown.

Just to note, looking in the cod for datevec I see in subfunc _date_vfmt2sfmt_ already 5 consecutive calls to regexprep and 3 to strrep. So those two I add can't make that much difference, can they.

Philip Nienhuis <philipnienhuis>
Project Member
Fri 30 Jun 2017 10:14:18 PM UTC, comment #15: 

Sure, my cset is a kludge but one that works for me.

First, I'm not so proficient in the internals of strptime and friends and when I delved into this bug it soon appeared to me that time zones and DST constitute a science on their own.
Furthermore apparently almost no one bothered about this bug for 5 years (the original bug report is from 2012); admittedly it is a corner case that few people hit.
Therefore I didn't delve into this all the way and fixed it just for my own purposes.

Initially the code overhead was also a concern for me, but after looking into datenum and datevec and finding how much code those functions need to do their job I was quite a bit less alarmed :-)

But I agree that some timings may be helpful.  In my own experience I didn't note significant performance hits.

Philip Nienhuis <philipnienhuis>
Project Member
Fri 30 Jun 2017 09:44:40 PM UTC, comment #14: 

Also your test is very locale / time-zone specific, will not actually test anything for US users for example. Test must set a specific time zone environment variable to be useful.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Fri 30 Jun 2017 09:42:58 PM UTC, comment #13: 

Adding 2 calls to regexprep and a call to strftime in the critical path of datevec seems like it might add some overhead to every datenum / datevec call, whether or not they are affect. I'm interested in timings with and without this patch.

When looking at bug #51340, I wondered if we might add a parameter to strptime to indicate that it should ignore the local time zone internally.

Another option might be to 'putenv ("TZ", "UTC0")' for the duration of this operation, and restore it to either unset or previous value. But I'm uncertain on the safety and raciness of setting / unsetting user environment variables for sections of code like this.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Wed 24 Aug 2016 06:26:31 AM UTC, comment #12: 

cset attached including test.

Admittedly it is a bit kludgy but the most reliable way I could come up with is to
- reconstruct the original time string with strftime from strptime's output, and
- compare that with the original string
while both have been stripped of spaces and zero digits, to get rid of a.o., leading zeros and extra spaces introduced by strftime (see below).

I've run "make check" with this cset applied to my local repo to check if any other date/time related output is affected; no FAILs were introduced by it so I consider this patch fairly safe :-)

IMO strptime isn't the most suited tool to parse date/time strings due to this DST issue, but as it is probably the only efficient tool available we'll have to make do with it.

BTW, I noted that the internal format string fed to strptime can sometimes be off in the sense that it adds leading zeros that weren't in he original date/time string, e.g., by using %H rather than %j; while in turn  %j introduces an extra space instead of a zero digit.
But as those internal formats aren't exposed I didn't investigate further.

(file #38336)

Philip Nienhuis <philipnienhuis>
Project Member
Tue 23 Aug 2016 09:52:22 PM UTC, comment #11: 

After some searching on the web I infer that it is implementation-dependent what strptime will do with "nonexistent" times, here meant to be a time in the skipped hour when changing to DST.
Apparently the strptime that Octave uses chose to translate such times to DST and implicitly add an hour.

I wonder how Matlab does it - that is able to convert series date/time strings around DST switch time, into a continuous and consecutive series of datenums. Apparently ML does not invoke strptime at all.

I've experimented a bit with strptime() and strftime() but it turns out very hard to circumvent strptime's behavior and detect if it has fiddled with the hour, w/o multiple calls to these functions,i.e., there's some performance penalty.

But maybe that is not such a big deal.

Philip Nienhuis <philipnienhuis>
Project Member
Tue 23 Aug 2016 03:17:19 PM UTC, comment #10: 

Delving a bit into it I conclude that it is actually strptime() (called by datevec.m in turn called by datenum.m) that is the culprit.

It looks like strptime() thinks that in the night of switching to DST, DST starts at 02:00 AM sharp.
The fix could simply be making strptime() interpret DST start time to be 3:00 AM rather than 2:00 AM.  After all, actual times between 2:00 AM and 3:00 AM (2:00 AM incl.) do not exist in the night of switching to DST in spring, so if a user supplies a date/time string with a time between 2:00 AM and 3:00 AM it is probably explicitly meant to be a non-DST time string.

The complementary bug might exist at the time of switching back in fall but there it is probably automatically neutralized. At least, I surmise that must be the reason that no errors show up at those times.

I'm not so proficient in C++ so I hope someone more knowledgeable in C++ and time.cc can fix this.

I had a look but I couldn't find where in time.cc the octave::sys::time t variable is actually created.

Philip Nienhuis <philipnienhuis>
Project Member
Mon 22 Aug 2016 07:11:17 PM UTC, comment #9: 

In bug #48858 (a duplicate of this one) I posted some examples.

After some investigation (see bug #48858) it turns out that is is only around the time in spring that DST starts (i.e., that one hour window) that datenum gives erroneous results. In the autumn when DST ends, datenum is OK.
So datenum "obeys" DST for one hour after DST start, after that it falls back to non-DST time.

datevec is OK AFAICS, so I'll adapt the title to only mention datenum

Matlab consistently returns 'winter time' datenums.

Octave-3.6.4 already had this bug.

(OS => any)

Philip Nienhuis <philipnienhuis>
Project Member
Wed 30 Apr 2014 08:33:53 AM UTC, comment #8: 

For my work, we've generally elected to use UTC timestamps on all our data to avoid issues with DST changes in local time.  I'd find it highly unfortunate if octave decided to try to enforce local time DST convention on me in such a case.  Note that while my data timestamps are in UTC, my computer clock is generally not.

Ben Kurtz <bkurtz>
Sun 19 Jan 2014 08:29:45 PM UTC, comment #7: 

Yes, Matlab produces the same results. This was done for me in TZ=EST5EDT.

>> num1 = 733112 + 1/12;
>> num2 = 733112 + 1/8;
>> datestr(num1)

ans =

11-Mar-2007 02:00:00

>> datestr(num2)

ans =

11-Mar-2007 03:00:00

>> datenum('11-Mar-2007 02:00:00') == num1

ans =

     1

>> datenum('11-Mar-2007 03:00:00') == num2

ans =

     1

>> datenum([2007 3 11 2 0 0]) == num1

ans =

     1

>> datenum([2007 3 11 3 0 0]) == num2

ans =

     1

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Thu 14 Mar 2013 07:04:12 PM UTC, comment #6: 

Can we have someone with access to Matlab verify whether datenum produces the same result when fed either a string or a datevector?

In Octave, the code forks.  A datevector is used to directly calculate a datenum.  If a string is presented to datenum it must first be converted to a datevector by calling datevec() and this starts to involve localtime() and other system specific features.

Rik <rik5>
Project Administrator
Thu 26 Jul 2012 03:23:49 PM UTC, comment #5: 

The behavior you see (both examples) seems to be incompatible with Matlab.

I don't know what the right behavior is regarding DST.

Is it documented that Matlab ignores DST?

John W. Eaton <jwe>
Project Administrator
Thu 26 Jul 2012 07:22:19 AM UTC, comment #4: 

My computer's locale is US, so Daylight Savings Time does explain the behavior I'm seeing. However, it is confusing that datenum returns different values for March 11, 2007 at 2:00am when it is expressed as a string and a vector.

code:

s = datenum('11-Mar-2007 02:00:00')
v = datenum([2007 3 11 2 0 0])

output:

s =  733112.125000000
v =  733112.083333333

Anonymous
Thu 26 Jul 2012 04:03:54 AM UTC, comment #3: 

I agree with Jordi, this is clearly related to a daylight savings time transition. On the days in question in your time zone, 2:00am is the same time as 3:00am. You will see similar results using the C library functions strptime and mktime with your sample time strings.

Does this explain what you are seeing or do you expect some different behavior?

I don't know what the expected behavior from Matlab is in these cases.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Thu 26 Jul 2012 04:01:03 AM UTC, comment #2: 

What is your computer's locale? Are you in Canada or the US? I think given DST, there was no such time, two in the morning of March 11, 2007, since at that instant time was advanced by one hour. If this matches your locale settings, I think Octave is behaving correctly.

Jordi GutiƩrrez Hermoso <jordigh>
Project Administrator
Thu 26 Jul 2012 03:54:41 AM UTC, comment #1: 

Confirmed on dev. It seems to be a daylight savings time issue. March 11, 2007 was the start of DST in the US and Canada.

Jordi GutiƩrrez Hermoso <jordigh>
Project Administrator
Thu 26 Jul 2012 02:42:58 AM UTC, original submission:  

I have noticed that the functions datenum and datevec incorrectly convert date strings. Specifically, a date string that represents 2:00am is converted to a serial date number and date vector as 3:00am. I have only seen this behavior for the hour of 2:00am in the month of March in the years 2007-2010. The day on which the error occurs depends on the year. In 2007, 2008, 2009 and 2010, the error occurs on March 11, 9, 8 and 14, respectively. The following example illustrates the error for March 11, 2007.

code:

for h=1:4
  s = sprintf('03/11/2007 %02d:00',h)
  n = datenum(s)
  v = datevec(s)
end

output:

s = 03/11/2007 01:00
n =  733112.041666667
v =

   2007      3     11      1      0      0

s = 03/11/2007 02:00
n =  733112.125000000
v =

   2007      3     11      3      0      0

s = 03/11/2007 03:00
n =  733112.125000000
v =

   2007      3     11      3      0      0

s = 03/11/2007 04:00
n =  733112.166666667
v =

   2007      3     11      4      0      0

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #41659:  tztest.m added by mtmiller (2KiB - text/x-objcsrc)
file #41629:  datevec.patch added by mtmiller (3KiB - text/x-patch)
file #41626:  datevec.patch added by mtmiller (3KiB - text/x-patch)
file #38336:  bug_36954_datevec_DST.cset added by philipnienhuis (2KiB - application/octet-stream)

 

Carbon-Copy List
  • -email is unavailable- added by avinoam (Posted a comment)
  • -email is unavailable- added by philipnienhuis
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by jordigh (Posted a comment)
  • -email is unavailable- added by None (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 25 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-09-01 mtmiller StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2017-08-29 mtmiller StatusPatch Submitted => Ready For Test
    2017-08-25 mtmiller Attached File- => Added tztest.m, #41659
    2017-08-23 mtmiller Assigned tophilipnienhuis => mtmiller
    2017-08-23 mtmiller Attached File- => Added datevec.patch, #41629
    2017-08-23 mtmiller Attached File- => Added datevec.patch, #41626
        StatusIn Progress => Patch Submitted
    2017-08-23 mtmiller StatusPatch Submitted => In Progress
    2017-08-23 mtmiller StatusIn Progress => Patch Submitted
    2017-08-23 avinoam StatusPatch Submitted => In Progress
    2017-06-30 mtmiller Dependencies- => bugs #51340 is dependent
    2017-06-30 mtmiller Summarydatenum converts date string incorrectly during DST start => datenum / datevec convert date-time string incorrectly during hour lost to DST
    2016-08-24 philipnienhuis Attached File- => Added bug_36954_datevec_DST.cset, #38336
        StatusConfirmed => Patch Submitted
        Assigned toNone => philipnienhuis
    2016-08-23 rik5 Carbon-CopyRemoved 72865 => -
    2016-08-22 philipnienhuis Carbon-Copy- => Added philipnienhuis
    2016-08-22 philipnienhuis Operating SystemGNU/Linux => Any
        Summarydatenum and datevec convert date string incorrectly => datenum converts date string incorrectly during DST start
    2016-08-22 mtmiller Dependencies- => bugs #48858 is dependent
    2014-01-19 mtmiller CategoryNone => Octave Function
    2012-11-23 jordigh Dependencies- => bugs #37798 is dependent
    2012-07-26 jordigh StatusNone => Confirmed
        Release3.2.4 => dev

    Back to the top


    Powered by Savane 3.5