ENH: Add GEOSHilbertCode to C API #556
Conversation
brendan-ward
changed the title
|
Note: I think that we don't actually need |
| ensure_equals(encoder.encode(geoms[i]->getEnvelopeInternal()), expected[i]); | ||
|
|
||
| // cleanup geom | ||
| factory_->destroyGeometry(geoms[i]); |
There was a problem hiding this comment.
I found something like this in one of the other tests, but I'm not confident this is the right way to handle this; without it there is a memory leak.
There was a problem hiding this comment.
That's fine. GEOSGeom_destroy is OK too. It's awkward because the interface of HilbertEncoder is forcing you to work with raw pointers.
src/shape/fractal/HilbertEncoder.cpp
Outdated
| @@ -65,7 +65,7 @@ HilbertEncoder::sort(std::vector<geom::Geometry*>& geoms) | |||
| bool | |||
| operator()(const geom::Geometry* a, const geom::Geometry* b) | |||
| { | |||
| return enc.encode(a->getEnvelopeInternal()) > enc.encode(b->getEnvelopeInternal()); | |||
| return enc.encode(a->getEnvelopeInternal()) < enc.encode(b->getEnvelopeInternal()); | |||
There was a problem hiding this comment.
@pramsey I changed the sort direction to better match expected results; std::sort returns items in ascending order, and the few other places I checked in GEOS that were sorting objects with a comparator like this were also doing so in ascending order. Presuming the descending Hilbert sort order was not intentional, of course.
I didn't see any other usage of this function in GEOS. Does this merit a note in the NEWS?
There was a problem hiding this comment.
It's possible nobody uses this, but anybody who does will be pretty pissed off. Is there any reason not to just leave it as is?
There was a problem hiding this comment.
Was only changing out of interest of consistency / expected results; since this PR added tests based on expected behavior, seemed like a good time to do so.
Definitely don't want to piss anyone off, just thought it was odd that a sort that normally would produce ascending order did the opposite here.
Would you like me to revert this change?
There was a problem hiding this comment.
Yes, I think so. One of the privileges of being a widely used library is being stuck with your history, particularly in the case of behaviour.
There was a problem hiding this comment.
The new C API function could do an ascending sort, no? Never mind, the new function is operating on a single input.
closes #553
Exposes
HilbertEncoderto C API.HilbertEncoderwill return seemingly valid values for geometries that fall outside the extent used for scaling prior to callingHilbertCode. Should this instead be handled as an error? For now I noted that it is the caller's responsibility to ensure the geometry falls in the extent.I used @jorisvandenbossche 's GeoPandas PR #2297 to validate the Hilbert Codes used in the test here, since both use the same calculations.