[Django] #30889: gis.measure: Distance/Distance should error.

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[Django] #30889: gis.measure: Distance/Distance should error.

Django
#30889: gis.measure: Distance/Distance should error.
---------------------------------------+------------------------
               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.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/048.80fc610c8a18c44cc44e94443f998437%40djangoproject.com.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30889: gis.measure: Distance/Distance should error.

Django
#30889: gis.measure: Distance/Distance should error.
-----------------------------+--------------------------------------
     Reporter:  Robert Coup  |                    Owner:  nobody
         Type:  New feature  |                   Status:  closed
    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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.bae1738b43b77dbe6a2f182d33f55322%40djangoproject.com.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30889: gis.measure: Distance/Distance should error.

Django
In reply to this post by Django
#30889: gis.measure: Distance/Distance should error.
-----------------------------+--------------------------------------
     Reporter:  Robert Coup  |                    Owner:  nobody
         Type:  New feature  |                   Status:  closed
    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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.9e2fc519aa30f7bd697e592f9a6cb103%40djangoproject.com.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30889: gis.measure: Distance/Distance should error.

Django
In reply to this post by Django
#30889: gis.measure: Distance/Distance should error.
-----------------------------+--------------------------------------
     Reporter:  Robert Coup  |                    Owner:  nobody
         Type:  New feature  |                   Status:  closed
    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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.0bb99521ade3e8e60a8b662418e898df%40djangoproject.com.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30889: gis.measure: Distance/Distance should error.

Django
In reply to this post by Django
#30889: gis.measure: Distance/Distance should error.
-----------------------------+--------------------------------------
     Reporter:  Robert Coup  |                    Owner:  nobody
         Type:  New feature  |                   Status:  closed
    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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.6ffaf346765c96ce394d7bfea41a35da%40djangoproject.com.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30889: gis.measure: Distance/Distance should error.

Django
In reply to this post by Django
#30889: gis.measure: Distance/Distance should error.
-----------------------------+--------------------------------------
     Reporter:  Robert Coup  |                    Owner:  nobody
         Type:  New feature  |                   Status:  closed
    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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.61861c8b88664e43c4a57e0a0aef3fd2%40djangoproject.com.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30889: gis.measure: Distance/Distance should error.

Django
In reply to this post by Django
#30889: gis.measure: Distance/Distance should error.
-----------------------------+--------------------------------------
     Reporter:  Robert Coup  |                    Owner:  nobody
         Type:  New feature  |                   Status:  closed
    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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.fc0544433a798f020e42e827f04dbf08%40djangoproject.com.
Reply | Threaded
Open this post in threaded view
|

Re: [Django] #30889: gis.measure: Distance/Distance should error.

Django
In reply to this post by Django
#30889: gis.measure: Distance/Distance should error.
-----------------------------+--------------------------------------
     Reporter:  Robert Coup  |                    Owner:  nobody
         Type:  New feature  |                   Status:  closed
    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>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

--
You received this message because you are subscribed to the Google Groups "Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To view this discussion on the web visit https://groups.google.com/d/msgid/django-updates/063.de69ddc9fc53f7d7a6785d3d6e95c8b9%40djangoproject.com.