Code

Opened 2 years ago

Closed 2 years ago

Last modified 16 months ago

#17754 closed Cleanup/optimization (fixed)

Django GIS Measure refactor

Reported by: riccardodivirgilio Owned by: nobody
Component: GIS Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

hi, the class MeasureBase in django.contrib.gis.measure is not subclassable.

i've write a new code, that allows the very same API, but it can be subclassed with no pain

the new code is one hundred line shorter, and is DRY.

in the old code the class Area and the class Distance write the same methods every time.

i've pushed a lot of base methods (like repr, init, sub, add, ...) in the base class, and i've defined just molt for the new Distance class and div for the new Area class.

Please consider to include this code in the django 1.4 version.

I've tested it and it works like the old version, but now you can create a new class for Weight with no method definition, just unit definition.

if you want i can write standard measure for every measure and we can put it inside localflavor...

Attachments (7)

measure.2.2.py (15.6 KB) - added by riccardodivirgilio 2 years ago.
Measure py with TEST
measure.diff (16.5 KB) - added by riccardodivirgilio 2 years ago.
Measure diff unified format
measure.2.py (15.6 KB) - added by riccardodivirgilio 2 years ago.
Measure py with TEST
measure.py (15.9 KB) - added by riccardodivirgilio 2 years ago.
full .py file
measure.2.diff (17.4 KB) - added by riccardodivirgilio 2 years ago.
measure_u.diff (3.5 KB) - added by riccardodivirgilio 16 months ago.
measure.3.diff (1.2 KB) - added by riccardodivirgilio 16 months ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 2 years ago by riccardodivirgilio

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

changed a bug with idiv

comment:2 Changed 2 years ago by xavierpencilline

pretty cool stuff!

comment:3 Changed 2 years ago by lukeplant

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

At a first glance, this seems like a sensible improvement.

It won't get in to Django 1.4 now - we will be prioritising bug fixes at this stage.

Also, we need a patch in unified diff format to be able to review more easily.

Thanks!

comment:4 Changed 2 years ago by riccardodivirgilio

ok in that case i'll have the time to improve it.

what i'm doing is to create Measure, Area and Volume class and add mol and div method to switch from one class to another.

i'll also add a Weight class, so maybe it would be better to put this measure class in another package (like localflavor) and then just import it from gis.

comment:5 Changed 2 years ago by riccardodivirgilio

please wait that i'll post a new version. there are some algebric issue with this class.

with this class you can't divide Measure with Measure, but in math this is wrong...

3m * 3m = (3*3) * (m *m) = 9sq_m

3m / 3m = (3/3) * (m/m) = 1

i'll attach a new version that support div with other instance of the same class

Last edited 2 years ago by riccardodivirgilio (previous) (diff)

comment:6 Changed 2 years ago by riccardodivirgilio

i've attached a new measure.py, with tests at the end.

just lunch python measure.2.py to test it

there is also the new diff file in unified format.

i've created also a Weight class to see how much it's easy to create a new measure class,

maybe it would be useful for developers to include a weight class somewhere in django

comment:7 Changed 2 years ago by riccardodivirgilio

Improvements are:

1: class names are printed in exceptions

2: div and mul with distance retruns correct instances of other classes (example 3m * 3m = 9sq_m

3: div with two instances of same class returns a number (example: 3m / 3m = 1)

feel free to choose the correct prefix for volume API, just change the string in the code.

todo: remove tests at the end.

Last edited 2 years ago by riccardodivirgilio (previous) (diff)

Changed 2 years ago by riccardodivirgilio

Measure py with TEST

Changed 2 years ago by riccardodivirgilio

Measure diff unified format

Changed 2 years ago by riccardodivirgilio

Measure py with TEST

comment:8 Changed 2 years ago by riccardodivirgilio

sorry i've done a mess with attachments...

comment:9 Changed 2 years ago by riccardodivirgilio

maybe it would be useful to use number_format when calling string, to easy use the class in django templates and i18n format number

def str(self):

return '%s %s' % (number_format(getattr(self, self._default_unit)), self._default_unit)

Changed 2 years ago by riccardodivirgilio

full .py file

Changed 2 years ago by riccardodivirgilio

comment:10 Changed 2 years ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

In [4d46106f8c233c5af283c903000ef8c6f1165393]:

Fixed #17754 -- Refactored gis.measure

This refactoring does allow much easier MeasureBase subclassing.
Many thanks to Ricardo di Virgilio for the initial patch.

comment:11 Changed 2 years ago by claudep

Riccardo, thanks for this improvement. I've reworked your patch and committed only the refactoring part. The Weight and Volume classes are a nice proof of concept about how easy it is to subclass MeasureBase, but I did not include them because I don't see a real need for them in Django core.

Feel free to request further improvements in separate tickets.

comment:12 Changed 16 months ago by riccardodivirgilio

Hi, i'm migrating my code to django 1.5

there is a problem with the measure class, every time you mix decimal and float it raises a type error

so i've made a small diff to convert every numeric type to float automatically.

check the diff

Changed 16 months ago by riccardodivirgilio

Changed 16 months ago by riccardodivirgilio

comment:13 Changed 16 months ago by claudep

Riccardo, I'd suggest opening a different ticket. Could you also provide tests for the issue (see django/contrib/gis/tests/test_measure.py)?

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.