Opened 12 years ago

Closed 12 years ago

Last modified 11 years ago

#17754 closed Cleanup/optimization (fixed)

Django GIS Measure refactor

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

Download all attachments as: .zip

Change History (20)

comment:1 by Riccardo Di Virgilio, 12 years ago

changed a bug with idiv

comment:2 by xavierpencilline, 12 years ago

pretty cool stuff!

comment:3 by Luke Plant, 12 years ago

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

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

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

comment:6 by Riccardo Di Virgilio, 12 years ago

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

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

by Riccardo Di Virgilio, 12 years ago

Attachment: measure.2.2.py added

Measure py with TEST

by Riccardo Di Virgilio, 12 years ago

Attachment: measure.diff added

Measure diff unified format

by Riccardo Di Virgilio, 12 years ago

Attachment: measure.2.py added

Measure py with TEST

comment:8 by Riccardo Di Virgilio, 12 years ago

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

comment:9 by Riccardo Di Virgilio, 12 years ago

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)

by Riccardo Di Virgilio, 12 years ago

Attachment: measure.py added

full .py file

by Riccardo Di Virgilio, 12 years ago

Attachment: measure.2.diff added

comment:10 by Claude Paroz <claude@…>, 12 years ago

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

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

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

by Riccardo Di Virgilio, 11 years ago

Attachment: measure_u.diff added

by Riccardo Di Virgilio, 11 years ago

Attachment: measure.3.diff added

comment:13 by Claude Paroz, 11 years ago

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