openeo-python-client icon indicating copy to clipboard operation
openeo-python-client copied to clipboard

Inconsistent and non-standard CRS handling for geometry arguments in DataCube

Open soxofaan opened this issue 1 year ago • 4 comments

stumbled on this while working on #104: https://github.com/Open-EO/openeo-python-client/blob/5a3e6f40cf5945ea885165db9845587435be66f0/openeo/rest/datacube.py#L1074-L1087

e.g. public facing from cube.aggregate_spatial(..., crs=...) and cube.mask_polygon(..., srs=...)

This CRS handling has some problems:

  • it's an ad-hoc, non-standard feature not present in official processes
  • it's inconsistent: sometimes it will apply (when geometry is client-side GeoJSON), sometimes it won't (e.g. back-end side vector cube)
  • it will ignore crs info already present in GeoJSON and will blindly overwrite it

I think we should get rid of this

soxofaan avatar Nov 27 '24 09:11 soxofaan

related discussions and threads:

  • https://github.com/Open-EO/openeo-processes/issues/235
  • #204
  • e344c4c105e8d5b13e16d99bc19baeb92ca79cf0
  • https://github.com/Open-EO/openeo-python-client/pull/202

soxofaan avatar Nov 27 '24 12:11 soxofaan

As far as I understand the threads above, the "crs" support mentioned above was originally targeted to the use case where the geometry is provided as shapely object (which has no explicit CRS info), which has to be converted to GeoJSON to be embedded in the process graph. Over time, a lot more geometry value types for geometries were added (e.g UDP parameters, vector cubes, load_url, ...), where it's impossible to attach that CRS to, so we now have a very inconsistent CRS handling situation.

soxofaan avatar Nov 27 '24 16:11 soxofaan

proposed solution:

  • stop supporting crs/srs arguments directly in aggregate_spatial, ... (because it's impossible to do it consistently). Instead throw exception pointing to proper way to do it (e.g. docs).
  • provide convenience function to support the conversion need from shapely to GeoJSON with custom CRS

So use cases that wanted to use shapely geometries with custom crs the old way:

cube = cube.aggregate_spatial(
    geometries=shapely_geometry,
    crs="EPSG:123456",
    ...
)

would have to change to something like

cube = cube.aggregate_spatial(
    geometries=shapely_to_geojson(shapely_geometry,  crs="EPSG:123456"),
    ...
)

soxofaan avatar Nov 27 '24 17:11 soxofaan

I would:

  • make sure that shapely geometry remains supported
  • add shapely geometry support to load_collection as well, the absence there is the most annoying inconsistency for me
  • improve documentation of crs keyword to state that it's actually deprecated.

jdries avatar Nov 27 '24 18:11 jdries