#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)
Change History (20)
comment:1 by , 13 years ago
comment:3 by , 13 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → 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 by , 13 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 , 13 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
comment:6 by , 13 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 , 13 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 = 3sq_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.
comment:9 by , 13 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 , 13 years ago
Attachment: | measure.2.diff added |
---|
comment:10 by , 13 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:11 by , 13 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 , 12 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 , 12 years ago
Attachment: | measure_u.diff added |
---|
by , 12 years ago
Attachment: | measure.3.diff added |
---|
comment:13 by , 12 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
)?
changed a bug with idiv