Aryeh Leib Taurog wrote:
> So I have extended the LineString and LinearRing classes' __getitem__
> and __setitem__ methods to allow slicing, added a __delitem__ while I
> was at it, and threw in most of the non-special, standard list methods
> as well.
> I'd like to do this for the geometry collections next, but I'm posting
> this here (is there a better place?) to solicit feedback on these mods
> before I continue.
> Questions
> 1. Is there some really good reason not to do this that I'm missing,
> or am I just the first one who wanted it enough to sit down and do it?
When I first created the GEOS interface, I wanted to have fully mutable
geometries. In fact, I tried to make the objects more memory efficient
by re-using geometries (e.g., keeping previous references to internal
rings if shell is replaced). This did not turn out well -- recycling
`const` pointers is a bad idea, leading to unpredictable behavior and
crashes.
From there I moved to a non-modification policy, with any geometry
modifications resulting in a new cloned geometry. This has turned out
to be a solid approach thus far.
The moral of the story is that as much as we want these things to behave
like Python objects, we have to remember it's really a thin layer over
the GEOS C++ objects -- and have to abide by its limitations or face
very serious consequences when things go awry.
> 2. Did I do this correctly? It seems to work. I tested the correctness
> of the mutations themselves, but haven't extensively tested the GEOS
> methods on the mutated objects.
Let me save any implementation commentary until I've applied and tested
myself. I've skimmed the patch, and it "looks" to be right (the
expansive test suite is appreciated and necessary for such a growth of
the API).
What I want to make sure of is that these methods don't create any
adverse side effects. Specifically, I want to avoid segmentation
faults. Unfortunately, these type of problems occur a lot with the type
of operations you're doing, e.g., copying a geom to a modified state and
deleting the original reference.
I'm not saying these bugs are present, but they can manifest themselves
in subtle ways, especially outside common usage scenarios. Thus, I'd
want very robust tests of these features, especially deletion, of corner
cases and strange user hijinks to lessen the risk of unanticipated crashes.
> 3. I've created a situation where
> del geometryObj[:] # now works
> geometryObj = LinearString([]) # doesn't work
> Is there some reason I shouldn't do that?
In the first statement, `del` is deleting what is retrieved in the slice
operation, in this case it would be clones -- so `geometryObj` itself
isn't actually deleted, and persists. In the current implementation, I
don't support 'empty' geometries (although the OGRGeometry
implementation does). This is a reflection of the underlying libraries
themselves as (if IIRC) GEOS doesn't let you create empty geometries and
OGR (GDAL) does.
> 4. Does it make sense to implement similar mutation methods for Point
> objects or Polygon objects?
I'm not sure -- as it is, you can just use `geom.coords`, get an array,
mutate as necessary and then use the modified arrays in the geometry
constructors.
> 5. I think it would be nice to make the LinearRing mutations fool-
> proof. Meaning since the first and last point must be the same, a
> LinearRing object with n points should really behave like a list of n
> - 1 coordinates, all the while maintaining the last coordinate as a
> mirror of the first so that the geometry won't be invalidated by the
> mutations. Comments?
This type of coordinate management is done at the GEOS level, and I'm
not sure about this level of implementation without a robust use case.
> 6. I implemented count() which is redundant with __len__() and, in
> this case, num_points(). Overkill?
Possibly -- I'm trying to keep the API parred down, especially since I
already have a lot of redundant methods (e.g., `exterior_ring`, `shell`).
> 7. Also I'm not sure if index(x) and remove(x) made sense in this
> context, but I could imagine cases where they might be useful and so I
> erred on the side of implementing as much of the standard list
> interface as I thought reasonable. Comments?
> 8. The sort() and reverse() methods don't seem applicable here. These
> are the only standard list methods which I did not implement. However,
> I think that they might apply to the geometry collections. Comments?
I think the `index` routine would be useful; but I'm hesitant about the
in-place modification routines (sort, reverse, remove) for the
aforementioned reasons about geometry modification.
Anyone else with comments?
-Justin
