Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#17754 closed Cleanup/optimization (fixed)

Django GIS Measure refactor

Reported by: Riccardo Di Virgilio 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 Riccardo Di Virgilio 5 years ago.
Measure py with TEST
measure.diff (16.5 KB) - added by Riccardo Di Virgilio 5 years ago.
Measure diff unified format
measure.2.py (15.6 KB) - added by Riccardo Di Virgilio 5 years ago.
Measure py with TEST
measure.py (15.9 KB) - added by Riccardo Di Virgilio 5 years ago.
full .py file
measure.2.diff (17.4 KB) - added by Riccardo Di Virgilio 5 years ago.
measure_u.diff (3.5 KB) - added by Riccardo Di Virgilio 4 years ago.
measure.3.diff (1.2 KB) - added by Riccardo Di Virgilio 4 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 5 years ago by Riccardo Di Virgilio

changed a bug with idiv

comment:2 Changed 5 years ago by xavierpencilline

pretty cool stuff!

comment:3 Changed 5 years ago by Luke Plant

Patch needs improvement: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/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 5 years ago by Riccardo Di Virgilio

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 5 years ago by Riccardo Di Virgilio

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 5 years ago by Riccardo Di Virgilio (previous) (diff)

comment:6 Changed 5 years ago by Riccardo Di Virgilio

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 5 years ago by Riccardo Di Virgilio

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 5 years ago by Riccardo Di Virgilio (previous) (diff)

Changed 5 years ago by Riccardo Di Virgilio

Attachment: measure.2.2.py added

Measure py with TEST

Changed 5 years ago by Riccardo Di Virgilio

Attachment: measure.diff added

Measure diff unified format

Changed 5 years ago by Riccardo Di Virgilio

Attachment: measure.2.py added

Measure py with TEST

comment:8 Changed 5 years ago by Riccardo Di Virgilio

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

comment:9 Changed 5 years ago by Riccardo Di Virgilio

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 5 years ago by Riccardo Di Virgilio

Attachment: measure.py added

full .py file

Changed 5 years ago by Riccardo Di Virgilio

Attachment: measure.2.diff added

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

Resolution: fixed
Status: newclosed

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 4 years ago by Claude Paroz

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 4 years ago by Riccardo Di Virgilio

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 4 years ago by Riccardo Di Virgilio

Attachment: measure_u.diff added

Changed 4 years ago by Riccardo Di Virgilio

Attachment: measure.3.diff added

comment:13 Changed 4 years ago by Claude Paroz

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

Note: See TracTickets for help on using tickets.
Back to Top