Opened 6 years ago

Closed 6 years ago

#3960 closed task (fixed)

Unify centroid functions

Reported by: komzpa Owned by: komzpa
Priority: medium Milestone: PostGIS 2.5.0
Component: postgis Version: master
Keywords: Cc:

Description

Code currently contains at least three centroid implementations:

ST_Centroid backing function, calls GEOS, has CIRCULAR* support, no weight support:

https://github.com/postgis/postgis/blob/b8400aeff39419e6e57f61589a66d3bc4f4a6cc5/postgis/lwgeom_geos.c#L1346

lwgeom_centroid, KMeans backing, calls GEOS, has no CIRCULAR* support visible:

https://github.com/postgis/postgis/blob/8d538e952743acf3e9311e4f576d2afd00ae1fc1/liblwgeom/lwgeom_geos.c#L940

median init_guess, only points, no GEOS, supports weights:

https://github.com/postgis/postgis/blob/svn-trunk/liblwgeom/lwgeom_median.c#L124

It looks like all three can be merged and pulled closer together, providing a weighted centroid for multipoints and support for CIRCULAR* in kmeans.

Are there any other places?

Change History (5)

comment:1 by dbaston, 6 years ago

#1 and #2 seem like good candidates to merge (add curve support to #2, and have #1 delegate to #2).

I'd keep #3 separate, as I don't think the use of M to signify "weight" is obvious or established throughout PostGIS.

comment:2 by komzpa, 6 years ago

Another centroid is in Geography.

Weighted by M, accepts multipoint (hey @dbaston there is such practice…): https://github.com/postgis/postgis/blob/64134491b6fad59a11da05d494b6046b715e00df/postgis/geography_centroid.c#L179 It looks like it doesn't accept Z and uses it only because of geog2cart transform.

The whole geography centroid file fully reimplements logic of geos centroid, if I'm correctly looking at its code here: https://github.com/OSGeo/geos/blob/master/src/algorithm/Centroid.cpp

Another small centroid in kmeans, 2D only, has logic of "check if it's current cluster" attached: https://github.com/postgis/postgis/blob/741b88eda71c80701366dffecd7fa665ee47060c/liblwgeom/lwkmeans.c#L45

GBOX centroid, uses fancy point iteration likely hardwired to memory layout: https://github.com/postgis/postgis/blob/b72778f0ff5564a6ec5b5e45620fc0e3b90c8433/liblwgeom/lwgeodetic.c#L258

comment:3 by pramsey, 6 years ago

Owner: changed from pramsey to komzpa

comment:4 by pramsey, 6 years ago

I'm reassigning this since it energizes me not at all.

comment:5 by komzpa, 6 years ago

Resolution: fixed
Status: newclosed

In 16608:

Unify geometry centroid functions

Make ST_Centroid call lwgeom_centroid.

Closes #3960
Closes https://github.com/postgis/postgis/pull/256

Note: See TracTickets for help on using tickets.