[Django] #30889: gis.measure: Distance/Distance should error.
Django
---------------------------------------+------------------------
Reporter: Robert Coup | Owner: nobody
Type: Bug | Status: new
Component: GIS | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------+------------------------
The original design principle of the measure module were to:
1. minimise unit mismatching errors
2. avoid bad maths on units.
3. provide helpful converters so everyone doesn't have constants all over
their codebases
With respect to (2), for example: (Area * Distance) is a meaningless thing
- m³
While `Distance / Distance` (& `Area / Area`) do produce ratios, they are
''quite likely'' to be an error.
Originally `Distance / Distance` would raise a `TypeError`, same as `Area
/ Area` still does currently. This was changed during a refactoring a
while back (#17754).
Suggestion to resolve:
1. Add a `MeasureBase.ratio(other)` method, which makes it really explicit
what is happening
2. Make `Distance / Distance` raises a `DeprecationWarning`, and
eventually go back to raising a `TypeError`
3. ~~Alias `ratio()` to another operator (eg. `%`)~~ I think it's not used
enough to actually be clearly readable to anyone.
--
Ticket URL: <https://code.djangoproject.com/ticket/30889>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Robert Coup | Owner: nobody
Component: GIS | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: new => closed
* resolution: => wontfix
* type: Bug => New feature
Comment:
If I understand correctly, you propose to remove a feature added in #17754
because you think it's unnecessary or maybe unclear. I don't see any
issues in supporting `Distance / Distance` or `Distance * Distance ` as
far as I'm concerned they work fine:
{{{
>>> d1 = Distance(m=100)
>>> d2 = Distance(km=1)
>>> d2/d1
10.0
>>> d1/d2
0.1
>>> d1 * d2
Area(sq_m=100000.0)
}}}
I also don't think that would be better (or more readable) to add a new
API (`ratio()`) for this.
--
Ticket URL: <https://code.djangoproject.com/ticket/30889#comment:1>
Django
Reporter: Robert Coup | Owner: nobody
Component: GIS | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Robert Coup):
> If I understand correctly, you propose to remove a feature added in
#17754 because you think it's unnecessary or maybe unclear.
Not saying that at all.
When I wrote measure originally, one goal was trying to make error-prone
handling of dimensioned values safer. That's the reasoning you can't
multiply `Distance * Area`, for example. The same reasoning you can't do
`MyModel.objects.delete()`, you need to explicitly use `.all().delete()`:
for safety.
It was changed in #17754 without any discussion that I can find, as part
of the bigger refactoring effort. I suggest the ticket is re-opened for
discussion as a bug.
> I don't see any issues in supporting Distance / Distance or Distance *
Distance as far as I'm concerned they work fine:
Yes, `Distance/Distance` returns a number, but it should raise a
TypeError. My suggestion was that if the user wants a ratio, then they
should have to explicitly ''ask'' for a ratio.
(`Distance * Distance` should definitely return an Area, that's the
simplest definition of an Area!)
Alternatively, if unit safety is no longer a goal of the module, then the
TypeError raised by `Area/Area` should be dropped, and arguably many of
the other TypeErrors, and the documentation should have a big "buyer
beware, we don't help prevent bad dimensioned maths anymore" statement
added.
--
Ticket URL: <https://code.djangoproject.com/ticket/30889#comment:2>
Django
Reporter: Robert Coup | Owner: nobody
Component: GIS | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
> Yes, Distance/Distance returns a number, but it should raise a
TypeError.
That's something that I'm missing, why it should raise an error? Do you
have examples of incorrect calculations?
> ... buyer beware...
Off-topic, as far as I'm concerned we don't have buyers :) .
--
Ticket URL: <https://code.djangoproject.com/ticket/30889#comment:3>
Django
Reporter: Robert Coup | Owner: nobody
Component: GIS | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* cc: Claude Paroz (added)
Comment:
Claude, Can you take a look?
--
Ticket URL: <https://code.djangoproject.com/ticket/30889#comment:4>
Django
Reporter: Robert Coup | Owner: nobody
Component: GIS | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Robert Coup):
>> Yes, Distance/Distance returns a number, but it should raise a
TypeError.
> That's something that I'm missing, why it should raise an error? Do you
have examples of incorrect calculations?
Sure, it prevents a class of programming errors for incompatible unit
conversions. The
[https://www.researchgate.net/publication/228991349_A_design_for_dimensional_analysis_in_robotics
original paper gis.measure was based on] discusses:
> ... Such a system should also support units, which give meaning to
dimensioned data. Units support also prevents inappropriate mixing of
incompatible dimensioned values, for example distance and time.
> Existing robot programming systems typically usefixed or floating point
numeric types to represent dimensioned data, particularly geometric data,
such as those in Table 1. Units are not specified, but an implied standard
unit is assumed and the programmer is responsible for ensuring all
necessary conversions.
> It is not safe to rely on the programmer to ensure unit conversions,
especially where different unit standards are used. In larger projects, the
difficulties are compounded since many programmers are involved. It is
difficult to change the implied standard units; the recent Player and Stage
unit change from millimetres to metres generated much discussion and
considerable reengineering of clients. Another, well-known incident was
the failure of NASA’s Mars Climate Orbiter mission; the root cause was
data with incorrect units (9).
> ...
> The dimensional analysis technique can help make programs more robust
against errors in units by verifying the consistency of the units given.
It allows a new class of errors [incompatible conversions] to be caught
(10). It can be applied by adding a new attribute to data types of the
dimensions and units used, enabling verification of data compatability and
automatic conversions.
> ...
> A key part of dimensional analysis is unit algebra: the interaction of
the units when dimensioned values are combined. It prevents incompatible
conversions (e.g. seconds to millimetres) while correctly transforming
appropriate combinations (e.g. metres divided by seconds gives a value in
metres per second). Unit algebra must be active for all dimensioned
values.
Now, `Unit / Unit` does produce a ratio, which can be useful in some
scenarios (and `Distance/Distance` does correctly return a float rather
than another Distance), though is often an accident. But whichever way it
goes we should be consistent across Distance & Area.
> ... buyer beware...
> Off-topic, as far as I'm concerned we don't have buyers :) .
The module did protect users, now it doesn't. IMO the same as if Django
decides to make `MyModel.objects.delete()` work — if the ORM allowed that
everything would "still work", but it would be a "bug" as a design
principle violation.
--
Ticket URL: <https://code.djangoproject.com/ticket/30889#comment:5>
Django
Reporter: Robert Coup | Owner: nobody
Component: GIS | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Hi Robert, thanks for your comments.
The change here was 7 years ago... "did protect users, now it doesn't"
seems to be stretching it. :)
The changes you describe were a deliberate design choice, not just an
unrelated result of the refactoring. (See e.g.
https://code.djangoproject.com/ticket/17754#comment:7)
As such we'd need a much fuller discussion than we can have on the issue
tracker. [https://docs.djangoproject.com/en/dev/internals/contributing
/triaging-tickets/#closing-tickets See the Triage Workflow for more].
For what it's worth here, I think that assuming that users know what
they're doing when do unit math is the right way to go.
Personally, I want to be able to use division when I need do. I neither
want nor need training-wheels that force me to go via a wrapper function.
(I might add such for learning, but even then, a big part of learning is
finding out, experimentally, when operations don't apply...)
I hope that makes sense.
Thanks again.
--
Ticket URL: <https://code.djangoproject.com/ticket/30889#comment:6>
Django
Reporter: Robert Coup | Owner: nobody
Component: GIS | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Robert Coup):
> The change here was 7 years ago... "did protect users, now it doesn't"
seems to be stretching it. :)
Ha! Didn't protect me yesterday, which is why I ended up here again ;-)
> The changes you describe were a deliberate design choice, not just an
unrelated result of the refactoring.
Good spot, I obviously missed (3) reading yesterday.
> For what it's worth here, I think that assuming that users know what
they're doing when do unit math is the right way to go.
Sure, I've said my piece.
Okay for a separate ticket & patch to remove the TypeError for
`Area/Area`? No reason for it to be inconsistent with Distance. I'll add a
brief note to the docs to explain what the module does/doesn't try and
help developers with as well.
{{{
In [4]: A(sq_m=100) / A(sq_m=10)
---------------------------------------------------------------------------
TypeError Traceback (most recent call
last)
<ipython-input-4-d578da15a196> in <module>
----> 1 A(sq_m=100) / A(sq_m=10)
/usr/local/lib/python3.7/site-packages/django/contrib/gis/measure.py in
__truediv__(self, other)
326 )
327 else:
--> 328 raise TypeError('%(class)s must be divided by a
number' % {"class": pretty_name(self)})
329
330
TypeError: Area must be divided by a number
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30889#comment:7>