-
Notifications
You must be signed in to change notification settings - Fork 16
Add enhanced filter_bbox with VectorCube support #329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Added VectorCube support to filter_bbox function - Fixed critical coordinate transformation bug in _reproject_bbox - Improved geometric filtering using intersects() instead of within() - Enhanced spatial filtering for both RasterCube and VectorCube types - Maintains backward compatibility with existing functionality Fixes: - Lines 280-310: Corrected bbox coordinate order and transformer parameters - Lines 267, 273: Changed geometric filtering to include boundary cases - Line 173: Updated return type annotation for Union support
Related to: https://github.com/EOEPCA/roadmap/issues/356 |
Great, just couple of things to do:
|
@Yuvraj198920 you should remove all the files added in the root folder and add a test under |
@Yuvraj198920 I see that you added a |
Per reviewer feedback (@clausmichele): - Merged filter_bbox.py functionality into existing _filter.py - Removed duplicate filter_bbox.py file - Removed demo scripts (demo_openeo_integration.py, test_netherlands_demo.py, test_vectorcube_example.py) - Kept Netherlands GeoJSON test data file Changes to _filter.py: - Added VectorCube support to filter_bbox function signature (Union[VectorCube, RasterCube]) - Added VectorCube filtering logic with intersects() method (includes boundary geometries) - Fixed critical coordinate transformation bug in _reproject_bbox: * Corrected bbox point order from [lat, lon] to [lon, lat] * Fixed transformer.transform call to use (x, y) with always_xy=True - Maintained full backward compatibility for RasterCube filtering All existing tests pass (5/5 in test_filter.py)
@Yuvraj198920 please add a test here: openeo-processes-dask/tests/test_filter.py Line 136 in a7eb91f
Let me know if you need help. |
@clausmichele hi, added test for filter_bbox_vectorcube. |
Thanks for the latest updates, I will have a look at the pr early next week! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the tests/data/Netherlands_polygon.geojson
required? I do not see it being used in the test?
Also, it would be good to include a test with a different crs being used to see if the _reproject_bbox
works as expected.
Thanks a lot for these updates!
…jection test - Remove tests/data/Netherlands_polygon.geojson (unused file) - Add test_filter_bbox_vectorcube_crs_reprojection() to validate _reproject_bbox function - Test uses VectorCube in UTM Zone 32N with WGS84 bbox to ensure CRS transformation works correctly - All filter tests passing (7/7)
|
Fixes: