-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
Unexpected truncation when storing float to int array #8733
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
Comments
Automatic conversion can be a very expensive operation if the array is large, so I'm definitely -1 on that. A warning seems pretty reasonable though, and it wouldn't surprise me if a mechanism already exists to do that. |
Thanks for the reply. Do you know where one would start looking in order to
implement such a warning?
On a philosophical note, we have two possible behaviors; one is logical
from the naive user's perspective, and the other is far cheaper. Is it best
to default to the user-friendly behavior and issue a warning about
inefficiency, or to default to the efficient behavior and issue a warning
about potential user misuse?
…On Thu, Mar 2, 2017 at 5:44 PM, Eric Wieser ***@***.***> wrote:
Automatic conversion can be a very expensive operation if the array is
large, so I'm definitely -1 on that. A warning seems pretty reasonable
though, and it wouldn't surprise me if a mechanism already exists to do
that.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#8733 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/APLYI6UGb4qFoPxkQWUTTE0lwi_vCwsoks5rh0ZLgaJpZM4MRn8N>
.
|
That choice is mostly irrelevant - the important thing is that it's best to default to the backwards-compatible behaviour. |
That makes sense. Thanks for the info!
…On Thu, Mar 2, 2017, 6:44 PM Eric Wieser ***@***.***> wrote:
That choice is mostly irrelevant - the important thing is that It's best
to default to the backwards-compatible behaviour.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#8733 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/APLYI6XgM0enzLVkruYLZZdjxypqveR5ks5rh1RdgaJpZM4MRn8N>
.
|
Ok, so here's where it's already implemented: >>> a=array([1,0])
>>> np.copyto(a[0,...], 1 + 1.1)
TypeError: Cannot cast scalar from dtype('float64') to dtype('int32') according to the rule 'same_kind'
|
Got it. I'm not familiar with Python.h, but I'm looking through the
multiarraymodule.c file and vaguely understanding what's going on in the
copyto function. It seems like the key line is 1782:
``` NPY_CASTING casting = NPY_SAME_KIND_CASTING;```
Now, do you know which function is called when I execute `a[0] = 1.1` ?
I.e., where the warning should be implemented?
…On Thu, Mar 2, 2017 at 6:56 PM, Eric Wieser ***@***.***> wrote:
Ok, so here's where it's already implemented:
>>> a=array([1,0])>>> np.copyto(a[0,...], 1 + 1.1)TypeError: Cannot cast scalar from dtype('float64') to dtype('int32') according to the rule 'same_kind'
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#8733 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/APLYI0Ybtr9SH17z_2ldw2UKlYKWgzWBks5rh1cxgaJpZM4MRn8N>
.
|
|
Digging deeper into
somewhere. I think if you did implement this, it should take the form of |
We should definitely raise an error for this. ISTR that this issue may have been reported before. |
I expect error would have backward compatibility issues --- which is why
complex->real casts don't raise either by default (you can make them
raise by using warnings module to catch Complexwarning).
|
Hmm, what's the end goal here? If I have this code
We want it to throw a warning, right? What should that warning encourage if this behaviour is deliberate? |
The idea is that most such occurrences will be programming errors. That was certainly the case for complex numbers. Being explicit also has the advantage that the conversion, truncation or rounding, will be clear. The code will also be easier to understand with an explicit cast. |
Right, but my point is that we have no way of specifying that cast without also doing a copy, do we? |
I think you can use functions for that, copyto for instance. The default conversion of floats is truncation. For inplace addition you can use the np.add with the out parameter, etc. |
For inplace ufunc, we already have the warning. Assignment is the only thing that is "unsafe" still. (though possibly its just a deprecation warning, don't remember the current state) |
|
Perhaps this is an indication that copyto should support advanced indexing.
Although I'm sure that's non-trivial and may have unintended consequences
elsewhere.
…On Fri, Mar 3, 2017, 2:09 PM Eric Wieser ***@***.***> wrote:
out and copyto only work for views, not advanced indexing
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#8733 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/APLYI9vsNCBUbpDfa5tF2FURs8tIjPdJks5riGVrgaJpZM4MRn8N>
.
|
Also, I guess I'm making assumptions here about what |
Depends a lot on the code paths taken, most involved ones end up doing much the same as copyto (or even call it for the view cases). Plus object arrays do get an extra copy in some cases. But no, normally there is no copy made. You could add indexing to copyto in principle. I am not quite sure I am convinced that this is the only right thing. OTOH, it is one of those traps learning numpy |
Ugh, I thought we had deprecated this. But @seberg's right, we seem to have gone all the way through the deprecation cycle for in-place operations, but missed simple assignment:
I agree with @charris et al that we should deprecate these unsafe conversions on assignments and eventually make them an error. |
I'm not sure whether this directly addresses the issue or if it's reasonable, but it just occured to me that there would be no ambiguity in my original situation if a=array([1,0]) defaulted to a float array. If that were the case, then somebody who intended to truncate the value they're saving in an int array would simply explicitly declare a=array([1,0],dtype=int64). Is there a reason this is not presently the case? Would it break backwards-compatability or be significantly less efficient for most use cases? |
Terrible compatibility break. The default is to use the minimum suitable kind compatible with the given elements. |
FWIW, Julia has always thrown an error when you try to implicitly assign a non-integer value to an integer-typed location: julia> a = [1,0]
2-element Vector{Int64}:
1
0
julia> a[1] = 1.1
ERROR: InexactError: Int64(1.1) and I haven't heard anyone say that they would prefer silent truncation. ( Deprecating (emitting a warning) then throwing an error seems like a good route for numpy too. |
Closing this, part of the discussion here predates gh-25621 by a long time, but it just seems like a question more anyway even back then. |
I was caught offguard by the following behavior using numpy 1.12:
Current behavior
Expected behavior
It may be that this is intentional behavior, but I would like to suggest automatic conversion of integer arrays to floating point arrays when a float is saved to an element or slice of the array. I'm not sure how straighforward implementation of this feature would be.
At the very least, there ought to be a warning when truncation occurs, especially since I at no point in the above code explicity declare the dtype of A to be int64, so I (and I think most users) would expect it to default to float64.
I'm interested in opinions on the matter.
Thanks,
Oliver
The text was updated successfully, but these errors were encountered: