10000 difference between behavior of assignment division and future division not documented (Trac #2061) · Issue #2653 · numpy/numpy · GitHub
[go: up one dir, main page]

Skip to content

difference between behavior of assignment division and future division not documented (Trac #2061) #2653

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
numpy-gitbot opened this issue Oct 19, 2012 · 12 comments

Comments

@numpy-gitbot
Copy link

Original ticket http://projects.scipy.org/numpy/ticket/2061 on 2012-02-21 by trac user AlanFrankel, assigned to @pv.

I just ran into the same issue described in bug 1734 (assignment division not compatible with future division). Namely, I expected to see that the results of

array_a /= scalar_s

would depend on whether a "from future import division" statement was included in the code. Eventually, I figured out this had no effect; the only way I could get true division, which I wanted, was to use:

array_a = array_a / scalar_s

I see that bug 1734 has been marked "won't fix". Whether or not I agree with that assessment, I definitely think that this behavior should be in user-accessible documentation (not just in the bug database). The "Mathematical Functions" page ( http://docs.scipy.org/doc/numpy/reference/routines.math.html ) doesn't have any reference to the assignment operators at all (+=, -=, *=, /=), which in my opinion is a glaring omission, especially since it's hard to search for these strings via a punctuation-stripping search engine. The assignment operators should be listed (or there should be a link to them), and the fact that "/=" gives different results from "/" should be stated.

@numpy-gitbot
Copy link
Author

@rgommers wrote on 2012-02-21

There is a big red warning box in the place I'd expect: http://docs.scipy.org/doc/numpy/reference/arrays.ndarray.html#arithmetic-and-comparison-operations

@numpy-gitbot
Copy link
Author

@mwiebe wrote on 2012-02-22

To add my opinion here: I want this case to raise an exception by default based on the 'same_kind' casting rule. e.g.

>>> a = np.array([1,2,3,4])
>>> a /= 1.5
>>> a
array([0, 1, 2, 2])
>>> np.divide(a, 1.5, out=a, casting='same_kind')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: ufunc 'divide' output (typecode 'd') could not be coerced to provided output parameter (typecode 'l') according to the casting rule 'same_kind'

@numpy-gitbot
Copy link
Author

trac user AlanFrankel wrote on 2012-02-22

In response to mwiebe: Would the behavior (generation of an error) differ depending on whether the division operator had been imported from future?

In response to rgommers: The red box is not where I expected it, is not very prominent on the page, and does not seem particularly relevant to the issue at hand at first glance. Nowhere on that page do the strings "+=", "*=, "/=" appear, for instance. In fact, when I search for "/=" from the NumPy search box, no documentation at all comes up. The connection between the double-underscore-decorated methods (e.g., ndarray.truediv) and the assignment operators is not obvious, and the fact that there are at least two issues at play (type coercion and truediv/div alternation) only makes the situation more confusing, especially in the absence of explanation. I won't have the time to come back to this immediately, but I am reopening it because I think the documentation in this area should be revisited.

@numpy-gitbot
Copy link
Author

@rgommers wrote on 2012-02-22

Mark, 'same_kind' behavior sounds reasonable to me (for 2.0).

@numpy-gitbot
Copy link
Author

@rgommers wrote on 2012-02-22

Alan, I don't have a problem with adding the explicit strings "/=", "+=", etc. to that section of the documentation if that would help. A patch on the doc wiki, github or here is welcome.

As to the location and visibility, we're just going to have to agree to disagree then. Here's my reasoning:

  • These things are called "in-place" operations, searching for that turns up the correct page.
  • A red warning box is not used often, and is about as visible as it gets.
  • The section "arithmetic operations" to me is a more logical place to look than the section "math routines" because, well, they're arithmetic operations.

IMHO this ticket can be closed again, but I'll leave it to someone else to do so.

@numpy-gitbot
Copy link
Author

@mwiebe wrote on 2012-02-22

With 'same_kind', there would be a slight difference, a consequence of how the types work.

Python 2 has int / int -> int, so array(int) /= array(int) is ok.

Python 3 has int / int -> float, so array(int) /= array(int) is trying to assign a float into an int array, and would raise an error.

@numpy-gitbot
Copy link
Author

@charris wrote on 2012-02-22

Already ran into the python 3 error in the ndimage tests, I assume it is enabled in current master.

@numpy-gitbot
Copy link
Author

@charris wrote on 2012-02-22

I think we can leave the ticket open for a bit in case Alan submits a patch.

Alan, it is generally considered rude to reopen a closed ticket, so you should probably apologize in advance when doing so ;) The mailing list is another good place to look for for a jury trial rather than summary judgment.

@numpy-gitbot
Copy link
Author

@mwiebe wrote on 2012-02-22

Yes, my memory failed me on this one. This was the particular case I had argued for:

http://mail.scipy.org/pipermail/numpy-discussion/2011-June/056605.html

This is just a small step towards tighter rules that protect programmers from "silly" errors. I would still argue in favour of keeping the change in, and adding information about it to the release notes. Clearly this should have gone in the release notes, with a special remark to trigger discussion before the release, but I didn't think of it, and the code review didn't flag it as missing either. Probably "The Checklist Manifesto" applies to situations like these.

#95

With Travis now more actively involved, I'm certain he would like the opportunity to examine the changes and weigh in.

@numpy-gitbot
Copy link
Author

@charris wrote on 2012-02-22

You must have missed one of the ndimage fixes, or else new tests were added, since another just went in ;)

I agree that tighter conversion rules are the way to go. There is a tradeoff between convenience and correctness, but in this case I think explicit use of '//' makes sense and leads to clearer code and fewer inexplicable results.

@numpy-gitbot
Copy link
Author

@mwiebe wrote on 2012-02-23

Looks like I was wrong again, and actually I did put it in the release notes. Here's what it says:

The default casting rule for UFunc out= parameters has been changed from 'unsafe' to 'same_kind'. Most usages which violate the 'same_kind' rule are likely bugs, so this change may expose previously undetected errors in projects that depend on NumPy.

@numpy-gitbot
Copy link
Author

@teoliphant wrote on 2012-02-23

I can support the "same_kind" casting approach for 2.0 and as a context-sensitive parameter in the 1.X series if we choose to spend the time on it. setitem will need the same work.

The current behavior can be confusing at first, but it is consistent with the "truncating" behavior of other things in NumPy (like setitem). There is an important difference between a = value and a = a value operation. The later allows for a complete change in the object that 'a' is bound to, while the former does not. Going to "same kind" can still make clear this difference but perhaps remove the most glaring surprises.

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

No branches or pull requests

1 participant
0