Flag to reduce precision of GeometryField #161
Conversation
README.rst
Outdated
| <https://docs.python.org/3/library/functions.html#round>`_), rounding values to | ||
| the provided level of precision. E.g. A Point with lat/lng of | ||
| ``[51.0486, -114.0708]`` passed through a ``GeometryField(precision=2)`` | ||
| would return a Point with a lat/lng of ``[51.04, -114.07]``. |
There was a problem hiding this comment.
You would think, however as I learned when writing this PR, 5 rounds down in Python 3. I know, right?
There was a problem hiding this comment.
Actually, this is probably a better reference (see the "note") section: https://docs.python.org/dev/library/functions.html#round
There was a problem hiding this comment.
That is correct if the original value is 51.0456. This indeed rounds down to 51.04. But the digit in question is '8' so 51.0486 definitely rounds up to 51.05.
There was a problem hiding this comment.
Oh, yeah, right you are...
In [1]: round(51.0486, 2)
Out[1]: 51.05Will fix that, thanks.
|
@auvipy @nemesisdesign Any input or feedback would be well appreciated. |
|
@RamizSami, could you please provide the WKT of the feature on question? I'll take a look. |
|
@RamizSami Have you been able to put together a test case illustrating the problem that you're having? Without seeing the data you're using and how you're using this library, I am unable to debug. |
|
@RamizSami Thanks for the data. We really only needed one geometry. In efforts to keep this PR tidy, could you do me a favour by deleting your past two comments? I uploaded 3 of your geometries here: https://gist.github.com/alukach/acfa950450d4c0a17026786941684a00 I made a test-case out of a simplified version of the first geometry and was able to use that to discover my error. It should be fixed now. Please pull the newest commit and test with your data again. @auvipy @nemesisdesign Feel free to provide any input. I believe this is good to go. |
|
@alukach deleted the comments. Also tested with the newest pull. It seems all good now |
|
@auvipy @nemesisdesign Ping, any thoughts or barriers to getting this merged? |
There was a problem hiding this comment.
Great work @alukach 👍, see below.
Why so many commented lines like:
# [-105.0215, 39.5771], # Dupe
Are these necessary?
rest_framework_gis/fields.py
Outdated
| def __init__(self, **kwargs): | ||
| def __init__(self, precision=None, remove_duplicates=False, **kwargs): | ||
| self.precision = precision | ||
| self.rm_dupes = remove_duplicates |
There was a problem hiding this comment.
I would really prefer if we could use readable instance attributes, method names and test method names as well, for consistency with the rest of the codebase
rest_framework_gis/fields.py
Outdated
| @@ -48,6 +62,39 @@ def validate_empty_values(self, data): | |||
| self.fail('required') | |||
| return super(GeometryField, self).validate_empty_values(data) | |||
|
|
|||
| def _recursive_round(self, val, precision): | |||
There was a problem hiding this comment.
could you find a better name for val?
rest_framework_gis/fields.py
Outdated
| return tuple(self._recursive_round(v, precision) for v in val) | ||
| return round(val, precision) | ||
|
|
||
| def _rm_redundant_points(self, geom, geotype): |
There was a problem hiding this comment.
can we use geometry and geo_type?
|
PS: once done cleaning up, please clean up the commit messages too, would be great to have 1 or a few commits at max |
|
@nemesisdesign
These are to identify to a developer that those values are the duplicated values from the source data that are removed from the response. I believe they will help keep those tests understandable and maintainable as time goes on.
Done, although I would recommend that in the future we use Github's Squash & Merge feature so that this is an automated step once the PR is approved. |
This is a continuation of #158 (I rebased the branch, making the PR ineligible for re-opening).
This is a feature proposal for adding flags to the
GeometryFieldto reduce the precision of a geometry. Perhaps the geometry is stored in high precision in the database, but for the sake of reducing response-size it may make sense to round the coordinates to a certain level.This features has two portions:
[ (1.1001, 1.2302), (1.1201, 1.2203), (1.1103, 1.240) ]to a precision value of1, the resultant array would be[ (1.1, 1.2), (1.1, 1.2), (1.1, 1.2)]. For the sake of size, it may as well be represented as[ (1.1, 1.2) ]if this were aMultiPointgeometry or[ (1.1, 1.2), (1.1, 1.2) ]if this were aLineStringgeometry.Tests and documentation are included within this PR. I have not upped the version of the library.