#19366 closed Bug (fixed)
GEOSIndexError when comparing geometries
Reported by: | Craig de Stigter | Owned by: | Claude Paroz |
---|---|---|---|
Component: | GIS | Version: | dev |
Severity: | Release blocker | Keywords: | |
Cc: | craig.destigter@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
In 1.5 (latest master) I get the following when comparing geometries of different lengths:
from django.contrib.gis.geos import * a = Polygon(((0, 0), (0, 1), (1, 1), (1, 0), (0, 0))) b = Polygon(((0, 0), (0, 1), (1, 0), (0, 0))) a < b Traceback (most recent call last): File "<string>", line 6, in <module> File "/home/cdestigter/checkout/django/django/contrib/gis/geos/mutable_list.py", line 164, in __lt__ c = self[i] < other[i] File "/home/cdestigter/checkout/django/django/contrib/gis/geos/mutable_list.py", line 164, in __lt__ c = self[i] < other[i] File "/home/cdestigter/checkout/django/django/contrib/gis/geos/mutable_list.py", line 80, in __getitem__ index = self._checkindex(index) File "/home/cdestigter/checkout/django/django/contrib/gis/geos/mutable_list.py", line 245, in _checkindex raise self._IndexError('invalid index: %s' % str(index)) django.contrib.gis.geos.error.GEOSIndexError: 'invalid index: 4'
This does not occur on django 1.4.x.
git bisect
tracks it down to this commit: https://github.com/django/django/commit/d04f72fb31bab43386e12963c1c6dec23ae16143 ("Got rid of old cmpmethods replaced by rich comparison.")
python 2.6.5, GEOS 3.3.3-CAPI-1.7.4
marking as release blocker since it's a regression in 1.5.
Attachments (4)
Change History (16)
comment:1 by , 12 years ago
Has patch: | set |
---|
comment:2 by , 12 years ago
Needs tests: | set |
---|---|
Owner: | changed from | to
Triage Stage: | Unreviewed → Accepted |
by , 12 years ago
Attachment: | 19366-1.diff added |
---|
comment:3 by , 12 years ago
Needs tests: | unset |
---|
Craig, can you check the patch?
I kept catching IndexError
, because it is possible that other
is a simple list or any other object which may raise a plain IndexError
.
I have also identified and fixed another regression in the equality method.
by , 12 years ago
Attachment: | 19366-2.diff added |
---|
comment:4 by , 12 years ago
My patch iterates over range(len(other))
to avoid getting an other._IndexError
(which may not be either IndexError
or self._IndexError
). I dropped the catching of IndexError
as it seems unnecessary in this case.
I also found another bug in __lt__
causing it to report incorrect results sometimes. I've added a test case to my patch to show the issue but haven't fixed the bug as I'm not quite sure the most efficient way to fix it.
comment:5 by , 12 years ago
Cc: | added |
---|---|
Patch needs improvement: | set |
by , 12 years ago
Attachment: | 19366-3.diff added |
---|
19366-2.diff plus fix lt when geometries are different lengths
comment:6 by , 12 years ago
After some thoughts, I think that comparing the linestrings for the equality makes sense, but not for >
or <
, because this will depend on the first differing point on the line string, which has absolutely nothing to do with being greater or smaller.
Shouldn't we override the __lt__
operator for polygons and compare the area instead?
comment:7 by , 12 years ago
Not necessarily. Calculating a polygon's area is pretty expensive, and if all you want is a *consistent* ordering for a list of geometries (eg for hashing them consistently), simply ordering based on the coordinates is probably a lot faster.
At the very least, changing it implies a backwards compatibility break for anyone using the old ordering.
by , 12 years ago
Attachment: | 19366-4.diff added |
---|
comment:8 by , 12 years ago
So I've added a documentation paragraph about Polygon comparison.
To solve the __lt__
issue, I don't think we can prevent using a second reverse comparison. Also done in that patch.
I also removed the import *
you added in previous patches, was this line intended?
comment:9 by , 12 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
Cool, that looks pretty good now. No, the import *
wasn't meant to be in the patch. I had trouble figuring out how to run the test_geos.py
tests, so that was my hackfix. Presumably there's a legit way to do it. They don't seem to run as part of the contrib.gis
tests :/
comment:10 by , 12 years ago
Thanks for the help and review.
Tests are run with gis
tests, however it is not possible to run individual tests or TestCases because of #6712.
comment:11 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
simple fix: https://github.com/koordinates/django/commit/751ebb7132de7d50a47114677892a3848de271b6