Skip to content

Conversation

@djkirkham
Copy link
Contributor

@djkirkham djkirkham commented Sep 5, 2016

In Numpy1.10, you cannot use in-place operators on arrays where the RHS cannot be upcast to the LHS, e.g., int += float

@@ -1,4 +1,4 @@
# (C) British Crown Copyright 2015, Met Office
# (C) British Crown Copyright 2015-2016, Met Office
Copy link
Member

@DPeterK DPeterK Sep 6, 2016

Choose a reason for hiding this comment

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

This needs to be formatted as 2015 - 2016 (note the spaces surrounding the dash -) as the current formatting is causing some of the license header tests to barf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@DPeterK
Copy link
Member

DPeterK commented Sep 6, 2016

In Numpy1.10, you cannot use in-place operators on arrays where the RHS cannot be upcast to the LHS

@djkirkham do you happen to have a reference for this assertion?

@djkirkham
Copy link
Contributor Author

@dkillick There's this section of the user guide. https://docs.scipy.org/doc/numpy-dev/numpy-user.pdf#page=13&zoom=auto,-266,577

@djkirkham djkirkham mentioned this pull request Sep 6, 2016
if min_sx < 0 and min_tx >= 0:
indices = np.where(sx_points < 0)
sx_points[indices] += modulus
sx_points[indices] = sx_points[indices] + modulus
Copy link
Member

Choose a reason for hiding this comment

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

In the cases where this change fires we are now going to change (i.e. upcast) the dtype of sx_points, yes? Is this desirable behaviour, even if it is unavoidable?

Alternatively, was the previous behaviour even correct? Or was it upcasting anyway and just not raising an exception when it did so?

Copy link
Contributor Author

@djkirkham djkirkham Sep 6, 2016

Choose a reason for hiding this comment

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

Yes, it will upcast. Thanks for flagging this up; I hadn't put enough thought into whether the change could have undesirable consequences. As it happens, I think we're ok: sx_points is a temporary array used only by this method. It appears to be used to find a list of indices, so the dtype won't proliferate into other arrays.

Previously, in the case where the dtype of the sx_points was initially int and modulus was a float, it would simply round modulus down to the nearest integer, which is clearly wrong.

Disregard the above, I was talking rubbish.

@djkirkham
Copy link
Contributor Author

Sorry this still isn't working...

@DPeterK
Copy link
Member

DPeterK commented Sep 7, 2016

@djkirkham I agree that this fixes the errors, thanks!

@DPeterK DPeterK merged commit c6db38a into SciTools:master Sep 7, 2016
@djkirkham djkirkham deleted the fix-array-add-ouput-dtypes branch September 7, 2016 09:29
@QuLogic QuLogic added this to the v2.0 milestone Sep 9, 2016
@QuLogic
Copy link
Member

QuLogic commented Oct 1, 2016

Can this be backported to v1.10.x?

@QuLogic QuLogic modified the milestones: v2.0, v1.11 Oct 7, 2016
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