Skip to content
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

Fix an overflow bug in decode_cf_datetime #2015

Merged
merged 5 commits into from
Mar 31, 2018
Merged

Conversation

fmaussion
Copy link
Member

@fmaussion fmaussion commented Mar 24, 2018

Not sure yet if this is the best way to do this.

@fmaussion
Copy link
Member Author

The problem comes from a weird numpy casting behavior. Consider:

In [1]: import numpy as np

In [2]: (np.int32(2) * np.int64(2)).dtype
Out[2]: dtype('int64')

In [3]: (np.array(np.int32(2)) * np.int64(2)).dtype
Out[3]: dtype('int64')

In [4]: (np.array([np.int32(2)]) * np.int64(2)).dtype
Out[4]: dtype('int32')

In the offending line:

flat_num_dates *  _NS_PER_TIME_DELTA[delta]

flat_num_dates is an array of dtype int32 and _NS_PER_TIME_DELTA[delta] is a scalar of dtype int64. Is there a way to make sure this line casts to a secure dtype better than my proposed solution to cast everything to a float64 first?

@fujiisoup
Copy link
Member

Thanks @fmaussion .

Is there a way to make sure this line casts to a secure dtype better than my proposed solution to cast everything to a float64 first?

How about np.array([np.int32(2)]) * np.array([np.int64(2)])?

I am not sure whether numpy explains this casting behavior, but I guess this is intentional.
This is consistent with

In [11]: (np.array([1.0], dtype=np.float32) * 1.0).dtype
Out[11]: dtype('float32')

where we expect the array dtype takes precedence.

@fujiisoup
Copy link
Member

I am curious why this test is failing in appveyor.
The casting rule of numpy should not depend on the operating system.

@fmaussion
Copy link
Member Author

I am curious why this test is failing in appveyor.
The casting rule of numpy should not depend on the operating system.

I'm curious too, but I don't have a windows machine to test it. The tests are green with the float64 casting though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants