BUG: Do not try sequence repeat unless necessary #7439
Conversation
Only if the other class actually implements sequence repeat, should it be tried. Otherwise classes which implement neither will cause the sequence repeat branch. This branch may fail for two reasons: 1. The scalar was not integer 2. The other object raises an error during repeat. Previously, the first did not happen for floats, etc. and the second was using a try/except logic. Now both will always error. An object which claims to support sequence repeat should never have to fall back to normal multiplication. Note that all of this is controversial in the final behaviour. We may actually want `[1, 2, 3] * np.float64(3.)` to return an array with `[3., 6., 9.]` at some point. - It was suggested to create a better error message, this is not covered here. The major point is fixed though, so: closing numpygh-7428
| npy_intp repeat; | ||
|
|
||
| /* | ||
| * If the other object supports sequence repeat and not number multiply | ||
| * we should call sequence repeat to support e.g. list repeat by numpy |
There was a problem hiding this comment.
Note that __repeat__ has been deprecated since 2.7.
|
LGTM modulo that it is doing what we want ;) If current behavior is truly a regression, might want to backport, although that makes me shiver a bit. @njsmith Thoughts? |
|
Current in 1.11 it only has spurrious warnings, which is annoying but not a regression. On Thu Mar 24 20:28:29 2016 GMT+0100, Charles Harris wrote:
|
|
OK, merged. Thanks Sebastian. I assume that this mostly clears things up. Do you think it needs a mention in the release notes? |
|
regarding backport: I guess technically there is a minor regression, because 1.10 gave an invisible warning for certain rare harmless actions, and 1.11 now gives a visible warning in the same case. Definitely too subtle of a fix to be sneaking into 1.11.0-final though :-/. I guess we could try releasing 1.11 as-is in the hopes that not many people will hit the spurious warning, and then if it turns out to be a problem... @arokem: as a representative of the one project we know of that's actually been affected so far... any thoughts? |
|
Not too important that I understand this, Just making sure that I understand what I am looking at, the line of code that raised the error for us (in #7428) is equivalent to test-case number 2 here ( |
|
@arokem: the question is whether it will bother you if 1.11.0 continues to issue a nonsensical warning on your original code |
|
Gotcha. I'm probably fine with that. We have quite a few warnings raised by our code, and we have generally been pretty lazy about fighting against it. |
Only if the other class actually implements sequence repeat, should
it be tried. Otherwise classes which implement neither will cause
the sequence repeat branch. This branch may fail for two reasons:
Previously, the first did not happen for floats, etc. and the
second was using a try/except logic. Now both will always error.
An object which claims to support sequence repeat should never
have to fall back to normal multiplication.
Note that all of this is controversial in the final behaviour.
We may actually want
[1, 2, 3] * np.float64(3.)to returnan array with
[3., 6., 9.]at some point.It was suggested to create a better error message, this
is not covered here. The major point is fixed though, so:
closing gh-7428