Can we somehow mark this and actually all classes as private API?
By exposing minimal new API, we have to worry less when making changes, and can minimize the new API to review carefully.

1. Should we return only one type? Input can indeed be polygon, but output is always rectangular tiles? I would however return them as polygon as well.
2. Many tiling systems also can assign a 'tile id', would be good if that is also returned

Ok, so split_area should then return a geopandas.GeoDataFrame with columns tile_id and geometry?

Do you have any suggestions for the tile_id convention for a SizeBasedTileGrid?

Here's an example precomputed grid, which also has tile id's:
https://artifactory.vgt.vito.be/artifactory/auxdata-public/grids/LAEA-20km-EU27.geojson
I would propose that this splitter should be able to replicate the same grid with the same tile id's.

return a geopandas.GeoDataFrame with ...

note that geopandas is also not a core dependency of the client. It's only a test dependency

For me personally, this is confusing: the unit of measure of UTM projection is meter, but we expect tile_size to be in kilometer.

FYI: pyproj is not a core dependency, only an optional one under the localprocessing extra at the moment.

Not sure if this dependency can be avoided, or is it a hard dependency for this whole module?

We indeed need to be able to reproject polygons in this module. Not sure if that's possible without pyproj? Possibly with geopandas, but that's also not a core dependency, and it built on pyproj I think

you can still do basic splitting without need for reprojection if you stay within the same CRS, right?
So I think it should be possible to keep pyproj an optional dependency

Note: in strict openEO, EPSG CRS descriptors are integer codes, instead of the string format "EPSG:XXX". String CRS descriptors are assumed to be WKT2.

e.g. see https://github.com/Open-EO/openeo-processes/blob/6141771129d58d2a292db3b91d80c812c49f4e52/load_collection.json#L67

Coordinate reference system of the extent, specified as as EPSG code or WKT2 CRS string.

 "anyOf": [
                                {
                                    "title": "EPSG Code",
                                    "type": "integer",
                                    "subtype": "epsg-code",
                                    "minimum": 1000,
                                    "examples": [
                                        3857
                                    ]
                                },
                                {
                                    "title": "WKT2",
                                    "type": "string",
                                    "subtype": "wkt2-definition"
                                }
                            ],

If we introduce something new in the client I think we should follow that convention from the start.
I'm not sure actually why I did that wrong in the aggregator

yes, we can not depend on aggregator from the client (the other way around is already happening, so that would actually be a circular dependency).
just remove this TODO

yes, we can not depend on openeo-python-driver from the client (the other way around is already happening, so that would actually be a circular dependency).
just remove this TODO

return a geopandas.GeoDataFrame with ...

note that geopandas is also not a core dependency of the client. It's only a test dependency

why only Polygon support and no MultiPolygon? I think we can support that here without any effort

as mentioned above: epsg codes should be annotated as integers (that's just what epsg codes are).
at best I would normalize string "EPSG:123" to int 123 .

e.g. check out openeo.util.normalize_crs

as mentioned above: I guess it's trivial to extend geometry support to MultiPolygons too

This looks like risky behavior if I understand correctly: in one case you reproject and in another you don't. I'm not sure users will anticipate that when naively using this function.
I understand you can document that, but these kind of important details are easily glossed over.

It's indeed assumed that the CRS of your Polygon is the same CRS as your SizeBasedTileGrid.

How would you suggest to detect the correct CRS of the Polygon?

at first sight I think this method tries to do too much (bboxes with crs, polygons without crs ..., reprojections, ...)

I'd extract the core functionality (double for-loop) as a helper and make its logic "CRS-free". And reuse that in more specialized versions (for CRS-aware bboxes, for naive polygons, ...)

Ok, so in this first version we'll just not reproject anything to avoid dependency on pyproj and instead stay in the same crs, which can be specified via:

• The crs key for a dict
• The projection argument in split_area for Polygons and MultiPolygons

Or what do you propose?

yes I would make sure to have a core function that does just tile splitting in a naive way (in the sense that it does not considers any CRS aspects). This function would then not have to depend on pyproj.
Then, reuse this function for particular uses cases that possibly require reprojects, which is fine as long as it does not leak into the core splitting function.
Not sure if it's clear what I mean :)

I guess it's a matter of taste, but these four fixtures mock_polygon_wgs, ... mock_dict_with_crs_utm feel like overkill to me.
I guess it is to be DRY, but in testing I think this extra level of indirection for these simple constructs is worse for readability than violating DRY.
Also, if you still to be DRY, I think you should just use constants instead of pytest fixtures (you are not really using them as fixtures, in terms of setup/teardown)

this if-else looks too simple

• epsg could be in upper case "EPSG:4326"
• not being "epsg:4326" is not enough to be sure it's UTM

also: the multiplication with 1000 looks quite ad-hoc and based on undocumented assumptions. In UTM the base unit is meters, but here you seem to assume I guess that the tile size is given in km, but that is currently not clear

• Do you have any recommended utils on how to detect if a certain CRS is in UTM?

• The expected units are indeed km for UTM (taken from the aggregator, can also just change it). It's documented in the split_area annotation, but I'll also add it to this class.

epsg codes in ranges 32601-32660 and 32701-32760 are UTM crses.
e.g. also see https://github.com/Open-EO/openeo-python-driver/blob/af50be3f59f2152ff36c15512735343fe7e463c5/openeo_driver/util/utm.py#L29-L33

The base unit of UTM is meter, so using something else (even if documented) is asking for trouble somewhere down the line. If you want to support working with kilometer to make it user friendly, you should make that explicit in some way (e.g. in the aggregator, the tiling specification is expected to contain an explicit unit like "km" : https://github.com/Open-EO/openeo-aggregator/blob/a0d40582c813a4d1ed62221389fd5a776106c809/src/openeo_aggregator/partitionedjobs/splitting.py#L80-L103 )

Ok, then I propose we just assume tile_size to be in meter for UTM crs