classification
Title: Bug in slice.indices()
Type: behavior Stage:
Components: Interpreter Core Versions: Python 2.6, Python 2.5
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: anakha, mark.dickinson, rhettinger
Priority: normal Keywords: patch

Created on 2008-05-29 18:35 by anakha, last changed 2008-06-20 14:57 by mark.dickinson. This issue is now closed.

Files
File name Uploaded Description Edit
slice.patch anakha, 2008-05-29 18:35 Patch to fix the problem
test_slice.patch anakha, 2008-06-18 15:55 Patch to add tests
issue3004.patch mark.dickinson, 2008-06-20 10:49
Messages (12)
msg67506 - (view) Author: Arnaud Bergeron (anakha) Date: 2008-05-29 18:35
When calling the indices method of a slice object with a negative stop
larger in absolute value than the length passed, the returned stop value
is always -1.  It should be 0 when the step is positive.

Current behavior:

>>> slice(-10).indices(11)
(0, 1, 1)
>>> slice(-10).indices(10)
(0, 0, 1)
>>> slice(-10).indices(9)
(0, -1, 1)
>>> slice(-10).indices(8)
(0, -1, 1)

Expected behavior:

>>> slice(-10).indices(11)
(0, 1, 1)
>>> slice(-10).indices(10)
(0, 0, 1)
>>> slice(-10).indices(9)
(0, 0, 1)
>>> slice(-10).indices(8)
(0, 0, 1)

The patch I submit trivially fixes this while preserving the expected -1
when the step is negative like in:

>>> slice(None, -10, -1).indices(8)
(7, -1, -1)

This issue affects python 2.5 and 2.6 at least.  I did not test with
other versions.
msg67714 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2008-06-05 14:24
I agree that those -1s should really be 0s.

Do you have any examples of real-life code
that's affected by this bug?  It doesn't seem
like something that would be a problem in
practice.
msg67733 - (view) Author: Arnaud Bergeron (anakha) Date: 2008-06-05 22:20
It's for code that I am developping.  I developped a class to allow
full slicing over iterators (like what islice does, but with negative
indexes).  When I have a positive step I just foward the call to
isclice using slice.indices() to compute my limits.  But with this
behavior, I am forced to wrap the indices() call with a function that
patches this case.

I agree that for the common usage of computing the limits of a for
loop it doesn't matter.  But it is really surprising when you realise
this behavior is what is causing these exceptions after having passed
half a day trying to debug your code.

In fact, I think this bug should really be more of a documentation bug
and I should propose a patch to add clearer documentation to this
method.  But the fix proposed should also go in because:
- it's trivial
- it's better

Documentation for the method will be submitted later tomorrow.  Should
I post another report or just attach it to this one?
msg67883 - (view) Author: Arnaud Bergeron (anakha) Date: 2008-06-10 01:00
Don't blame me for the delay, I have long days (yes, really up to 96
hours long :)

As for the documentation patch, I'm not certain anymore about it. 
Unless I bloat the description to about one full screen worth of text,
there will always be surprises for the user.  And describing only one
little part in detail feels inconsistent.

So unless I'm just a really bad writer, I think the current doc could be
left as-is.  I'm still all for the attached patch.  Comments anyone?  Or
does this needs more nagging?
msg68342 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2008-06-17 21:09
Could you provide some tests for the fixed behaviour?

I'll try to check this in (with appropriate tests) after the beta.
msg68366 - (view) Author: Arnaud Bergeron (anakha) Date: 2008-06-18 15:55
Would these do?

self.assertEqual(slice(None,   -10    ).indices(10), (0,  0,  1))
self.assertEqual(slice(None,   -11,   ).indices(10), (0,  0,  1))
self.assertEqual(slice(None,   -12, -1).indices(10), (9, -1, -1))

If yes, test_slice.patch adds them.
msg68448 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2008-06-20 09:59
On Wed, Jun 18, 2008 at 4:56 PM, Arnaud Bergeron <report@bugs.python.org>
wrote:

>
> Arnaud Bergeron <abergeron@gmail.com> added the comment:
>
> Would these do?
>
> self.assertEqual(slice(None,   -10    ).indices(10), (0,  0,  1))
> self.assertEqual(slice(None,   -11,   ).indices(10), (0,  0,  1))
> self.assertEqual(slice(None,   -12, -1).indices(10), (9, -1, -1))
>

Perfect.  Thank you.

If this is changed, then I think the following should also be
changed:

(9, 10, -1)

I believe the second index here should be 9, not 10.
Do you agree?

With these two changes the code, while marginally
more complicated, is actually easier to understand than
before, since exactly the same processing is applied
to both the start and stop indices.  I think this is as
it should be.
msg68449 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2008-06-20 10:01
Sorry: looks like I messed up that last post.  The example should be:

>>> slice(10, 10, -1).indices(10)  # expect (9, 9, -1)
(9, 10, -1)
msg68450 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2008-06-20 10:49
Here's a new patch that incorporates Arnaud's fix and tests, together with 
a few extra tests.

While I expect that this change will affect very little code, I think it's 
the right thing to do, because:

 - start and stop are now processed identically, making the source code
   easier to understand, and the behaviour easier to explain.

 - expected invariants now hold even in corner cases;  for example, after:

start, stop, step = my_slice.indices(length)

it's guaranteed that

0 <= start <= stop <= length    if step is positive, and
length-1 >= start >= stop >= -1 if step is negative.

However, I'd like a second opinion from another core developer before 
checking this in.
msg68451 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2008-06-20 10:52
> 0 <= start <= stop <= length    if step is positive, and
> length-1 >= start >= stop >= -1 if step is negative.

That should be:

0 <= start <= length and 0 <= stop <= length  (step > 0), and
length-1 >= start >= -1, length-1 >= stop >= -1 (step < 0);

it's not guaranteed that start <= stop always holds in the first case,
or that start >= stop in the second.
msg68458 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2008-06-20 13:10
Looks like a straight-forward patch.
msg68462 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2008-06-20 14:57
Committed, r64426.

Thanks for the report, Arnaud.
History
Date User Action Args
2008-06-20 14:57:17mark.dickinsonsetstatus: open -> closed
messages: + msg68462
2008-06-20 13:10:47rhettingersetassignee: rhettinger -> mark.dickinson
resolution: accepted
messages: + msg68458
2008-06-20 11:07:03rhettingersetassignee: mark.dickinson -> rhettinger
nosy: + rhettinger
2008-06-20 10:52:58mark.dickinsonsetmessages: + msg68451
2008-06-20 10:49:16mark.dickinsonsetfiles: + issue3004.patch
messages: + msg68450
2008-06-20 10:01:23mark.dickinsonsetmessages: + msg68449
2008-06-20 09:59:59mark.dickinsonsetfiles: - unnamed
2008-06-20 09:59:11mark.dickinsonsetfiles: + unnamed
messages: + msg68448
2008-06-18 15:55:46anakhasetfiles: + test_slice.patch
messages: + msg68366
2008-06-17 21:09:27mark.dickinsonsetassignee: mark.dickinson
messages: + msg68342
2008-06-10 01:01:00anakhasetmessages: + msg67883
2008-06-05 22:21:11anakhasetmessages: + msg67733
2008-06-05 14:24:42mark.dickinsonsetpriority: normal
nosy: + mark.dickinson
messages: + msg67714
2008-05-29 18:35:54anakhacreate