Datetime cleanup (mostly for Python 3) #161
Conversation
…d of STRING array on Python3
Done by making the unit given by datetime_data be a string instead of bytes in Python 3.
This includes warnings about shadowed variables, some bad casts, and others. Fixing these warnings also fixed a memory leak in one instance.
|
Looks good to me. |
|
Thanks for the review! |
| /* Create the output string data type with a big enough length */ | ||
| /* Get a string size long enough for any datetimes we're given */ | ||
| strsize = get_datetime_iso_8601_strlen(local, unit); | ||
| #if defined(NPY_PY3K) |
There was a problem hiding this comment.
Would it be possible to factor this out, to avoid defines inside function bodies ? I.e.
#ifdef NPY_PY3K
void foo()
{
// some code here
}
#endif
rather than
some_large_function(...)
{
....
#ifdef NPY_PY3K
// some code here
#endif
}
I know this is not strictly followed in numpy codebase, but it would be nice to avoid too much of this IMO.
There was a problem hiding this comment.
I've thought a bit about how to do this, but splitting this part out feels very artificial and doesn't seem like it would improve the clarity of the code at all. I'd rather keep it inline as it is. It's an artifact of the design choice to make NPY_STRING arrays not return strings in Python 3, something that's probably too late to change.
|
I noticed an interesting feature in the np.datetime64 construction (on Linux): but specifying a timezone makes it loop around: |
|
The Y2038 bug seems to be restricted to 32-bit Linux as far as I can tell. On Windows, there is datetime64_t, which I believe all your code changes still use, correct? In searching, it appears as if glibc didn't follow Microsoft in adding 64-bit variants of these important functions, and reading comments on forums reveals some perspectives like "all computers will be 64-bit before 2038 rolls around, so you shouldn't have to worry about it." The hack I've thought of is to shift years later than 2038 to before 2038, do the OS-specific stuff, then add the year offset back in once it's in NumPy's hands again. |
|
Looking at your specific example where you specify the timezone, I wouldn't expect that to fail, since it shouldn't be passing through a time_t in that case. Does it fail with the "Z" time zone as well? edit: The fail point would be converting to a local time for printing, so hopefully this works with the Y2038 hack I've added. |
This only seems to be cropping up on 32-bit Linux, where time_t is 32 bits, and there appears to be no 64-bit time_t extension like on Windows.
Also incorporated part of Han Genuit's pull request numpy#23, to try and reduce the difficulty dealing with the merge conflict that will result from these concurrent changes
|
What do people think is a reasonable way to deal with the issue on Windows, where the standard library doesn't want to apply timezones to dates before 1970? Should the 'Z' timezone be always used for dates prior to 1970 on that platform, or on all platforms? |
…nd later, because of Win32 On MS Windows, dates earlier than 1970 cause mktime to error. This patch also applies to parsing the same Y2038 hack that was done for printing.
|
I've restricted local parsing to dates >= 1970 as I suggested, and applied the Y2038 hack I proposed to both printing and parsing. I don't have platforms to test this on, so will need help with that! |
|
Thanks for the fixes! It tests OK on Linux (32bit). (Except for an unrelated polyfit issue.) Hopefully, the 2038 hack wont give problems with calendar conversions, but it might be the best solution on 32bit for now.. |
|
I've merged this in aded70c..10ed90c. |
feat: Add vabaq_[s8|s16|s32|u8|u16|u32]
This set of patches fixes up the datetime functionality to pass all tests in Python 3.1 and 3.2.