Skip to content

FIx to _array_find_type #107

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

Closed

Conversation

walshb
Copy link
Contributor

@walshb walshb commented Jul 10, 2011

Hi folks

This code was failing:

    dt = np.datetime64('1970-01-01', 'M')
    arr = np.array([dt])
    assert_equal(arr.dtype, np.dtype('M8[M]'))

It turns out that _array_find_type with no explicit supertype tries to find the common supertype of every list element and bool. This worked for datetime until commit 25bcc65 changed _npy_type_promotion_table.

I think the right thing is to stop this "default bool" behaviour.

Cheers

Ben

@charris
Copy link
Member

charris commented Jul 10, 2011

Heh. Looks like the correct diagnosis, I'll let Mark review the fix.

@mwiebe
Copy link
Member

mwiebe commented Jul 10, 2011

It looks great, thanks for the patch! Would you be willing to add tests for/verify that Python's datetime.date and datetime.datetime objects work in this context as well?

@walshb
Copy link
Contributor Author

walshb commented Jul 10, 2011

Oh alright then :-) Will add something to the commit soon...

@walshb
Copy link
Contributor Author

walshb commented Jul 12, 2011

Hi

Can I just check what you mean by "datetime.datetime objects work in this context"? Do we want np.array to always convert datetime.datetime to np.datetime64?

This would mean that np.array([datetime.date(2000, 1, 1)])[0].year will now break, as we don't have a datetime.date object any more; we have a np.datetime64.

So I'm not sure what to do. The "aggressive" conversion to datetime64 fits better with other numpy types, but it will mean more breakages of existing code. Perhaps I misunderstood your intentions here.

Cheers

Ben

@mwiebe
Copy link
Member

mwiebe commented Jul 12, 2011

That is what I was thinking, but I definitely see your point about how it breaks compatibility. I've been toying with ideas about how to expose the datetime as fields, but there doesn't seem to be a reasonable way to do it that would match Python's datetime.

At the very least, the behavior with regard to datetime.date and datetime.datetime should be well-defined, so we still need tests that confirm the current behavior if we're leaving it as is.

@walshb
Copy link
Contributor Author

walshb commented Jul 12, 2011

OK, I've added the test. I didn't create a branch (doh) so I'm having trouble attaching another commit to this request. Will submit a new request.

@mwiebe
Copy link
Member

mwiebe commented Jul 12, 2011

To get your new commits to show up, you have to push your branch to origin. In this case, you would push master to origin, but having a topic branch is better, for sure.

I've merged the two patches you made, thanks!

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