Repository navigation
point splitter - #954
point splitter#954HansVRP wants to merge 4 commits into
Conversation
b52cd8a to
022c383
Compare
|
@VincentVerelst can you take a look? |
|
@soxofaan could you have a look as well (I do still need to fix the unit tests) |
soxofaan
left a comment
soxofaan
left a comment
There was a problem hiding this comment.
can't go deep here, just some quick observations
| centroids: gpd.GeoSeries, | ||
| idx, | ||
| max_points: Optional[int], | ||
| out: List, |
There was a problem hiding this comment.
having the output as argument instead of return value looks like an ugly matlab-ism to me 😜
I guess it's to do the recursiveness , but I think that can also be done with a regular returns here
There was a problem hiding this comment.
yes indeed, it was done for recursive usage, can clean it up
| if "geometry" not in points.columns: | ||
| raise JobSplittingFailure("The GeoDataFrame must contain a 'geometry' column.") | ||
| if not points.geom_type.isin(["Point", "MultiPoint"]).all(): | ||
| raise JobSplittingFailure("All geometries must be of type Point or MultiPoint.") |
There was a problem hiding this comment.
I'm not sure it is necessary to enforce this constraint that all geometries must be points.
Lower you are using something like .centroid to get a point regardless of the original geometry type, so the logic would handle other types of geometry just fine, right?
There was a problem hiding this comment.
I mean, recursive quadrant based batching also seems interesting to have on a collection of polygons
There was a problem hiding this comment.
would indeed be pssible; but then we should change the naming as well. I was working on a points splitter, but it should rather become a geometry splitter then?
There was a problem hiding this comment.
point or geometry splitting sound like you are splitting the geometries themselves
maybe something along the lines of quandrant_grouping or quandrant_batching is more self-explanatory
There was a problem hiding this comment.
🟡 Changes recommended
Input features can be duplicated or lost, and custom tile grids do not satisfy the advertised interface contract.
3 open issues
What changed in this PR
Adds point-based job splitting to the existing job-management utilities, supporting sample extraction workflows alongside area-based splitting.
Changes:
- Introduces
split_pointswith optional feature limits, spatial subdivision, and tile-grid grouping. - Exports the helper publicly and documents its usage.
- Adds tests for supported inputs, grouping, reprojection, and validation.
| File | Description |
|---|---|
| tests/extra/job_management/test_job_splitting.py | Tests point splitting and tile-grid behavior. |
| openeo/extra/job_management/_job_splitting.py | Implements point grouping and recursive subdivision. |
| openeo/extra/job_management/__init__.py | Exports split_points publicly. |
| docs/cookbook/job_manager.rst | Adds point-extraction batching examples. |
| CHANGELOG.md | Announces the new helper. |
Review effort: Balanced
| if not points.geom_type.isin(["Point", "MultiPoint"]).all(): | ||
| raise JobSplittingFailure("All geometries must be of type Point or MultiPoint.") |
| if points.empty: | ||
| return [] | ||
|
|
||
| centroids = points.geometry.centroid |
| else: | ||
| raise JobSplittingFailure(f"Unsupported tile grid type: {type(tile_grid).__name__}.") |
|
FYI: any unit test failures you see about jsonschema are fixed with #960, so make sure to merge master to get rid of that problem |
While working on the ML blueprint I figured it should not be hard to extend the spitting function towards point geometries as well.
@VincentVerelst you are quite familiar with this; can you please chekc and validate as well?
For me the results look good and respect tilegrids as well: