[Django] #18887: LineString array method (property) returns different data type without and with NumPy installed
Django
with NumPy installed
----------------------------+------------------------------------
Reporter: mal | Owner: nobody
Type: Bug | Status: new
Component: GIS | Version: 1.4
Severity: Normal | Keywords: GIS, NumPy, LineString
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+------------------------------------
Apparently LineString (and probably other geometry types) return data in
different types depending on whether NumPy is installed.
Simple test case (reproducible in Django 1.3, 1.3.1 and 1.4.1).
Clean virtual env with only Django installed (and ipython).
First - no NumPy installed.
{{{
In [1]: from django.contrib.gis.geos import LineString
In [2]: line = LineString((0, 0), (3, 3))
In [3]: line.array
Out[3]: [(0.0, 0.0), (3.0, 3.0)]
}}}
Now - install NumPy and try again.
{{{
In [1]: from django.contrib.gis.geos import LineString
In [2]: line = LineString((0, 0), (3, 3))
In [3]: line.array
Out[3]:
array([[ 0., 0.],
[ 3., 3.]])
}}}
[(0.0, 0.0), (3.0, 3.0)] =! array([[ 0., 0.],[ 3., 3.]])
This is rather serious issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/18887>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
with NumPy installed
Type: Bug | Status: new
Component: GIS | Version: 1.4
Keywords: GIS, NumPy, | Triage Stage:
LineString | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by claudep):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Replying to [ticket:18887 mal]:
> This is rather serious issue.
Maybe you could add a real use case demonstrating why you consider this as
a serious issue?
--
Ticket URL: <https://code.djangoproject.com/ticket/18887#comment:1>
Django
with NumPy installed
Type: Bug | Status: new
Component: GIS | Version: 1.4
Keywords: GIS, NumPy, | Triage Stage:
LineString | Unreviewed
Has patch: 0 | Needs documentation: 0
Comment (by anonymous):
Replying to [comment:1 claudep]:
> Replying to [ticket:18887 mal]:
> > This is rather serious issue.
>
> Maybe you could add a real use case demonstrating why you consider this
as a serious issue?
Because it returns two different values depending on if a module is
installed or not? Any conditionals on the result will be dependant on if
numpy is installed or not.
--
Ticket URL: <https://code.djangoproject.com/ticket/18887#comment:2>
Django
with NumPy installed
Type: Bug | Status: new
Component: GIS | Version: 1.4
Keywords: GIS, NumPy, | Triage Stage: Accepted
LineString | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by claudep):
* stage: Unreviewed => Accepted
Comment:
Accepting on the base that this should be at least documented.
I see the potential issues, but apart documentation, do you see any
possible improvements?
--
Ticket URL: <https://code.djangoproject.com/ticket/18887#comment:3>
Django
with NumPy installed
Type: Bug | Status: new
Component: GIS | Version: 1.4
Keywords: GIS, NumPy, | Triage Stage: Accepted
LineString | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: deprince@… (added)
Comment:
Would it be an option to make numpy a dependency for the installation of
gis?
--
Ticket URL: <https://code.djangoproject.com/ticket/18887#comment:4>
Django
with NumPy installed
Type: Bug | Status: new
Component: GIS | Version: 1.4
Keywords: GIS, NumPy, | Triage Stage: Accepted
LineString | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Adam DePrince <deprince@…>):
Scratch that ... you don't want numpy to be required to install django.
It seems if the user wants a list they would call list(LineString(...))
instead of LineString(...).array. Perhaps we should issue a warning when
the user calls .array without numpy installed?
{{{
>>> line = LineString((0,0), (3,3))
>>> line.array
array([[ 0., 0.],
[ 3., 3.]])
>>> list(line)
[(0.0, 0.0), (3.0, 3.0)]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18887#comment:5>
Django
with NumPy installed
Reporter: mal | Owner: Ubercore
Component: GIS | Version: 1.4
Keywords: GIS, NumPy, | Triage Stage: Accepted
LineString | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* owner: nobody => Ubercore
--
Ticket URL: <https://code.djangoproject.com/ticket/18887#comment:6>
Django
with NumPy installed
Reporter: mal | Owner: Ubercore
Component: GIS | Version: 1.4
Keywords: GIS, NumPy, | Triage Stage: Accepted
LineString | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Ubercore):
I've confirmed this, going to look into it a little more closely to see if
we can make the behavior predictable somehow, without losing the ability
to take advantage of numpy if installed.
--
Ticket URL: <https://code.djangoproject.com/ticket/18887#comment:7>
Django
with NumPy installed
Reporter: mal | Owner: Ubercore
Component: GIS | Version: 1.4
Keywords: GIS, NumPy, | Triage Stage: Design
LineString | decision needed
Changes (by Ubercore):
* cc: Ubercore (added)
* stage: Accepted => Design decision needed
Comment:
Yeah, the only reasonable options I see are the ones outlined: warning
when array is called, and/or noting this in the docs. The behavior here
seems to be intended, not a bug. Marking as design decision needed.
--
Ticket URL: <https://code.djangoproject.com/ticket/18887#comment:8>
Django
with NumPy installed
Reporter: mal | Owner: Ubercore
Component: GIS | Version: 1.4
Keywords: GIS, NumPy, | Triage Stage: Design
LineString | decision needed
Has patch: 0 | Needs documentation: 0
Comment (by Adam DePrince <deprince@…>):
Here's a proposed fix if a warning in the absence of numpy is desired.
https://github.com/adamdeprince/django/commit/d02431ac5f15616df4cb3d39592651e59fdde17f
--
Ticket URL: <https://code.djangoproject.com/ticket/18887#comment:9>
Django
with NumPy installed
Reporter: mal | Owner: Ubercore
Component: GIS | Version: 1.4
Severity: Normal | Resolution:
LineString | Needs documentation: 0
-------------------------------------+-------------------------------------
Changes (by Ubercore):
* status: new => assigned
* has_patch: 0 => 1
* stage: Design decision needed => Accepted
Comment:
I've submitted a pull request for the following commit, which adds a
setting:
[https://github.com/peterlandry/django/commit/0c9839355a005e8805467aa09d4fcf9795965b76]
--
Ticket URL: <https://code.djangoproject.com/ticket/18887#comment:10>
Django
with NumPy installed
Reporter: mal | Owner: Ubercore
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: GIS, NumPy, | Triage Stage: Ready for
LineString | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Ubercore):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/18887#comment:11>
Django
with NumPy installed
Reporter: mal | Owner: Ubercore
Type: Bug | Status: assigned
Keywords: GIS, NumPy, | Triage Stage: Ready for
LineString | checkin
Has patch: 1 | Needs documentation: 0
Comment (by Adam DePrince <deprince@…>):
No, this doesn't capture what is really happening.
There is a bug with this: [(0.0, 0.0), (3.0, 3.0)] =! array( 0., 0.],[ 3.,
3.?)
This snippet doesn't mean
{{
not ([(0.0, 0.0), (3.0, 3.0)] =! array( 0., 0.],[ 3., 3.?))
}}
it means these types are completely incomparable.
If you compare these two you get completely different behavior than if you
compared two lists.
numpy.array([(0.0, 0.0), (3.0, 3.0)]) == [(0.0, 0.0), (3.0, 3.0)]
array([[ True, True],
[ True, True]], dtype=bool
There will be two populations that use this API. One will have numpy
installed on their dev machines and one won't. When they release
extensions and share code those they share it with may not have the same
numpy status.
It seems pretty clear that if you want a list you should call
list(LineString(...)) and if you actually want a numpy array you could
call .array and expect it. If the system will silently break that promise
you should have an exception, but we don't want to break existing code, so
I'm proposing we raise a warning instead. This diff doesn't do this.
My proposed code does.
We can't have two APIs that depend on the module you have installed; I
really feel strongly that .array without numpy installed should warn.
--
Ticket URL: <https://code.djangoproject.com/ticket/18887#comment:12>
Django
with NumPy installed
Reporter: mal | Owner: Ubercore
Type: Bug | Status: assigned
Keywords: GIS, NumPy, | Triage Stage: Ready for
LineString | checkin
Has patch: 1 | Needs documentation: 0
Comment (by Adam DePrince <deprince@…>):
https://github.com/django/django/pull/353
--
Ticket URL: <https://code.djangoproject.com/ticket/18887#comment:13>
Django
with NumPy installed
Reporter: mal | Owner: Ubercore
Type: Bug | Status: assigned
Severity: Normal | Resolution:
LineString | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* stage: Ready for checkin => Accepted
Comment:
Please do not mark your own patch as RFC.
--
Ticket URL: <https://code.djangoproject.com/ticket/18887#comment:14>
Django
with NumPy installed
Reporter: mal | Owner: Ubercore
Cleanup/optimization | Version: 1.4
Component: GIS | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: GIS, NumPy, | Needs documentation: 1
LineString | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by ptone):
* needs_better_patch: 0 => 1
* type: Bug => Cleanup/optimization
* needs_docs: 0 => 1
Comment:
I'm inclined to agree with claudep here that this is primarily a
documentation issue - an admonition probably.
The Numpy related LineString methods aren't documented at all.
A warning would become annoying if you've accepted the non-numpy behavior
in your code, and don't plan on changing it. This is as valid as the
Numpy approach as there has been no documentation. So why should that
user now suffer eternal warnings?
The comment on the property suggests that .array was to return a numpy
array, but all the other properties are commented as returning a "list or
numpy array"
If the numpy difference is documented clearly - anyone wanting to right
portable/sharable code can check {{{from django.contrib.gis.geos.base
import numpy}}} and make sure they standardize the data.
I'm closing the current pull requests as the wrong direction - not RFC.
--
Ticket URL: <https://code.djangoproject.com/ticket/18887#comment:15>
Django
with NumPy installed
Reporter: mal | Owner: sir-
Type: | sigurd
Cleanup/optimization | Status: assigned
Component: GIS | Version: 1.4
Severity: Normal | Resolution:
Keywords: GIS, NumPy, | Triage Stage: Accepted
LineString |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by sir-sigurd):
* owner: Ubercore => sir-sigurd
Comment:
I don't see any benefit in returning numpy array from Django, because
Django gets the list at first and then just creates numpy array with that
list, users can do it easily by themselves. So I propose to deprecate
`array` property in favor of `list(LineString)`.
--
Ticket URL: <https://code.djangoproject.com/ticket/18887#comment:16>
Django
with NumPy installed
Reporter: mal | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: GIS | Version: 1.4
Severity: Normal | Resolution:
Keywords: GIS, NumPy, | Triage Stage: Accepted
LineString |
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Changes (by Mariusz Felisiak):
* owner: Sergey Fedoseev => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/18887#comment:17>