Skip to content

Fixed kernel multiplication with numpy floats #2174#2652

Merged
embray merged 1 commit intoastropy:masterfrom
adonath:fix_for_#2174
Jul 24, 2014
Merged

Fixed kernel multiplication with numpy floats #2174#2652
embray merged 1 commit intoastropy:masterfrom
adonath:fix_for_#2174

Conversation

@adonath
Copy link
Copy Markdown
Member

@adonath adonath commented Jun 23, 2014

This PR fixes the multiplication of kernels with numpy floats. The bug was not easy to spot. The problem was that the __rmul__ method of the kernel was not called, because the kernel has an __array__ method implemented, which was used for the multiplication with numpy floats from the left. This behaviour is fixed now, by additionally implementing the __array_wrap__ method, which checks for the context of the multiplication. Furthermore I added some regression tests.

@cdeil
Copy link
Copy Markdown
Member

cdeil commented Jun 23, 2014

This fixes #2174 so 👍 to merge this before making the 0.4 branch.

I didn't look in detail how it works, but would it be useful to add a test that covers line 193 in __array_wrap__?
https://coveralls.io/files/226293580#L186

@astrofrog
Copy link
Copy Markdown
Member

It looks like float16 isn't available in Numpy 1.5 - can you add a workaround for that version?

@cdeil
Copy link
Copy Markdown
Member

cdeil commented Jun 23, 2014

I've never seen anyone use float16 and the code is not type specific, i.e. shouldn't have a problem with a specific dtype... should be OK to just remove it from the test, no?

@adonath
Copy link
Copy Markdown
Member Author

adonath commented Jun 23, 2014

As @cdeil suggested I removed np.float16 from the tests. It shouldn't make a difference, because the value, that is multiplied with, is only checked for being scalar, not a specific type. Travis builds have now passed.

@astrofrog astrofrog added this to the v0.4.1 milestone Jul 16, 2014
@astrofrog
Copy link
Copy Markdown
Member

@adonath - can you add a changelog entry?

@embray
Copy link
Copy Markdown
Member

embray commented Jul 23, 2014

@adonath Nag about adding a changelog entry under the 0.4.1 section so I can merge this. If you don't have time I can maybe do it though.

@adonath
Copy link
Copy Markdown
Member Author

adonath commented Jul 24, 2014

@embray Thanks for the reminder. I've added the changelog entry and rebased. Travis builds have passed.

@embray
Copy link
Copy Markdown
Member

embray commented Jul 24, 2014

Thanks @adonath . Any more comments on this, @cdeil ? Does it look good?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry to nitpick this, but it looks like you've inserted tabs. Please just insert spaces such that it lines up the same way as other entries.

@cdeil
Copy link
Copy Markdown
Member

cdeil commented Jul 24, 2014

👍

embray added a commit that referenced this pull request Jul 24, 2014
Fixed kernel multiplication with  numpy floats #2174
@embray embray merged commit 20a55eb into astropy:master Jul 24, 2014
embray added a commit that referenced this pull request Aug 6, 2014
Fixed kernel multiplication with  numpy floats #2174
@adonath adonath deleted the fix_for_#2174 branch February 10, 2015 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants