diff --git a/CHANGELOG.md b/CHANGELOG.md index d475e0781..ddcdb6a4e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Improve default handling of `crs` (and `base`/`height`) in `filter_bbox`: avoid explicitly sending `null` unnecessarily + ([#233](https://github.com/Open-EO/openeo-python-client/pull/233)). +- Update documentation/examples/tests: EPSG CRS in `filter_bbox` should be integer code, not string + ([#233](https://github.com/Open-EO/openeo-python-client/pull/233)). + + ### Removed ### Fixed diff --git a/openeo/imagecollection.py b/openeo/imagecollection.py index ecd565aca..8ddccbb50 100644 --- a/openeo/imagecollection.py +++ b/openeo/imagecollection.py @@ -9,7 +9,7 @@ from openeo.rest.job import RESTJob from openeo.rest.service import Service -from openeo.util import get_temporal_extent, first_not_none +from openeo.util import get_temporal_extent, first_not_none, dict_no_none if hasattr(typing, 'TYPE_CHECKING') and typing.TYPE_CHECKING: @@ -92,9 +92,8 @@ def filter_bbox(self, west, east, north, south, crs=None, base=None, height=None # Subclasses are expected to implement this method, but for bit of backwards compatibility # with old style subclasses we forward to `bbox_filter` # TODO: replace this with raise NotImplementedError() or decorate with @abstractmethod - kwargs = dict(west=west, east=east, north=north, south=south, crs=crs) - if base or height: - kwargs.update(base=base, height=height) + kwargs = dict(west=west, east=east, north=north, south=south) + kwargs.update(dict_no_none(crs=crs, base=base, height=height)) return self.bbox_filter(**kwargs) @deprecated(reason="Use `filter_bbox()` instead.") diff --git a/openeo/rest/datacube.py b/openeo/rest/datacube.py index 146834945..eaa1e7463 100644 --- a/openeo/rest/datacube.py +++ b/openeo/rest/datacube.py @@ -295,7 +295,8 @@ def filter_bbox( >>> cube.filter_bbox(west=3, south=51, east=4, north=52, crs=4326) - - With a (west, south, east, north) list or tuple:: + - With a (west, south, east, north) list or tuple + (note that EPSG:4326 is the default CRS, so it's not nececarry to specify it explicitly):: >>> cube.filter_bbox([3, 51, 4, 52]) >>> cube.filter_bbox(bbox=[3, 51, 4, 52]) @@ -365,10 +366,7 @@ def filter_bbox( raise ValueError(bbox) extent = {'west': west, 'east': east, 'north': north, 'south': south} - if crs is not None: - extent["crs"] = crs - if base is not None or height is not None: - extent.update(base=base, height=height) + extent.update(dict_no_none(crs=crs, base=base, height=height)) return self.process( process_id='filter_bbox', diff --git a/openeo/rest/imagecollectionclient.py b/openeo/rest/imagecollectionclient.py index 0d8b9ccaa..1733007d7 100644 --- a/openeo/rest/imagecollectionclient.py +++ b/openeo/rest/imagecollectionclient.py @@ -13,7 +13,7 @@ from openeo.rest import BandMathException from openeo.rest.job import RESTJob from openeo.rest.service import Service -from openeo.util import get_temporal_extent, legacy_alias +from openeo.util import get_temporal_extent, legacy_alias, dict_no_none if hasattr(typing, 'TYPE_CHECKING') and typing.TYPE_CHECKING: # Imports for type checking only (circular import issue at runtime). `hasattr` is Python 3.5 workaround #210 @@ -121,12 +121,8 @@ def _filter_temporal(self, start: str, end: str) -> 'ImageCollection': ) def filter_bbox(self, west, east, north, south, crs=None, base=None, height=None) -> 'ImageCollection': - extent = { - 'west': west, 'east': east, 'north': north, 'south': south, - 'crs': crs, - } - if base is not None or height is not None: - extent.update(base=base, height=height) + extent = {'west': west, 'east': east, 'north': north, 'south': south} + extent.update(dict_no_none(crs=crs, base=base, height=height)) return self.graph_add_process( process_id='filter_bbox', args={ diff --git a/openeo/util.py b/openeo/util.py index d2f751714..08c45853a 100644 --- a/openeo/util.py +++ b/openeo/util.py @@ -179,7 +179,7 @@ def date_to_rfc3339(d: Any) -> str: return rfc3339.normalize(d) -def dict_no_none(*args, **kwargs): +def dict_no_none(*args, **kwargs) -> dict: """ Helper to build a dict containing given key-value pairs where the value is not None. """ diff --git a/tests/data/0.4.0/aggregate_zonal_path.json b/tests/data/0.4.0/aggregate_zonal_path.json index d4f6f2bbf..75e7e555c 100644 --- a/tests/data/0.4.0/aggregate_zonal_path.json +++ b/tests/data/0.4.0/aggregate_zonal_path.json @@ -9,8 +9,7 @@ "west": 3, "east": 6, "north": 52, - "south": 50, - "crs": "EPSG:4326" + "south": 50 } }, "result": false diff --git a/tests/data/0.4.0/aggregate_zonal_polygon.json b/tests/data/0.4.0/aggregate_zonal_polygon.json index e9c87edf6..fcdd5361c 100644 --- a/tests/data/0.4.0/aggregate_zonal_polygon.json +++ b/tests/data/0.4.0/aggregate_zonal_polygon.json @@ -9,8 +9,7 @@ "west": 3, "east": 6, "north": 52, - "south": 50, - "crs": "EPSG:4326" + "south": 50 } }, "result": false diff --git a/tests/data/1.0.0/aggregate_zonal_parameter.json b/tests/data/1.0.0/aggregate_zonal_parameter.json index e1bc72ab9..a490c2670 100644 --- a/tests/data/1.0.0/aggregate_zonal_parameter.json +++ b/tests/data/1.0.0/aggregate_zonal_parameter.json @@ -17,8 +17,7 @@ "west": 3, "east": 6, "north": 52, - "south": 50, - "crs": "EPSG:4326" + "south": 50 } } }, diff --git a/tests/data/1.0.0/aggregate_zonal_path.json b/tests/data/1.0.0/aggregate_zonal_path.json index 998a8bd10..29aaf784c 100644 --- a/tests/data/1.0.0/aggregate_zonal_path.json +++ b/tests/data/1.0.0/aggregate_zonal_path.json @@ -17,8 +17,7 @@ "west": 3, "east": 6, "north": 52, - "south": 50, - "crs": "EPSG:4326" + "south": 50 } } }, diff --git a/tests/data/1.0.0/aggregate_zonal_polygon.json b/tests/data/1.0.0/aggregate_zonal_polygon.json index 09a18eb4e..f537fa8d5 100644 --- a/tests/data/1.0.0/aggregate_zonal_polygon.json +++ b/tests/data/1.0.0/aggregate_zonal_polygon.json @@ -17,8 +17,7 @@ "west": 3, "east": 6, "north": 52, - "south": 50, - "crs": "EPSG:4326" + "south": 50 } } }, diff --git a/tests/rest/datacube/test_datacube.py b/tests/rest/datacube/test_datacube.py index dce191ce4..28879c47f 100644 --- a/tests/rest/datacube/test_datacube.py +++ b/tests/rest/datacube/test_datacube.py @@ -17,10 +17,9 @@ from openeo.rest import BandMathException from openeo.rest.datacube import DataCube from openeo.rest.imagecollectionclient import ImageCollectionClient -from openeo.rest.service import Service +from .conftest import API_URL from .. import get_download_graph from ..conftest import reset_graphbuilder -from .conftest import API_URL from ... import load_json_resource @@ -219,60 +218,96 @@ def ndvi_scaled(cube, in_max=2, out_max=3): } -def test_filter_bbox(s2cube): +def test_filter_bbox_minimal(s2cube): + im = s2cube.filter_bbox(west=3.0, east=3.1, north=51.1, south=51.0) + graph = _get_leaf_node(im) + assert graph["process_id"] == "filter_bbox" + assert graph["arguments"]["extent"] == {"west": 3.0, "east": 3.1, "north": 51.1, "south": 51.0} + + +def test_filter_bbox_crs_4326(s2cube): + im = s2cube.filter_bbox(west=3.0, east=3.1, north=51.1, south=51.0, crs=4326) + graph = _get_leaf_node(im) + assert graph["process_id"] == "filter_bbox" + assert graph["arguments"]["extent"] == {"west": 3.0, "east": 3.1, "north": 51.1, "south": 51.0, "crs": 4326} + + +def test_filter_bbox_crs_32632(s2cube): im = s2cube.filter_bbox( - west=652000, east=672000, north=5161000, south=5181000, crs="EPSG:32632" + west=652000, east=672000, north=5161000, south=5181000, crs=32632 ) graph = _get_leaf_node(im) assert graph["process_id"] == "filter_bbox" assert graph["arguments"]["extent"] == { - "west": 652000, "east": 672000, "north": 5161000, "south": 5181000, "crs": "EPSG:32632" + "west": 652000, "east": 672000, "north": 5161000, "south": 5181000, "crs": 32632 } def test_filter_bbox_base_height(s2cube): im = s2cube.filter_bbox( - west=652000, east=672000, north=5161000, south=5181000, crs="EPSG:32632", + west=652000, east=672000, north=5161000, south=5181000, crs=32632, base=100, height=200, ) graph = _get_leaf_node(im) assert graph["process_id"] == "filter_bbox" assert graph["arguments"]["extent"] == { - "west": 652000, "east": 672000, "north": 5161000, "south": 5181000, "crs": "EPSG:32632", + "west": 652000, "east": 672000, "north": 5161000, "south": 5181000, "crs": 32632, "base": 100, "height": 200, } +@pytest.mark.parametrize(["kwargs", "expected"], [ + ({}, {}), + ({"crs": None}, {}), + ({"crs": 4326}, {"crs": 4326}), + ({"crs": 32632}, {"crs": 32632}), + ({"base": None}, {}), + ({"base": 123}, {"base": 123}), + ({"height": None}, {}), + ({"height": 456}, {"height": 456}), + ({"base": None, "height": 456}, {"height": 456}), + ({"base": 123, "height": 456}, {"base": 123, "height": 456}), +]) +def test_filter_bbox_default_handling(s2cube, kwargs, expected): + im = s2cube.filter_bbox(west=3, east=4, south=8, north=9, **kwargs) + graph = _get_leaf_node(im) + assert graph["process_id"] == "filter_bbox" + assert graph["arguments"]["extent"] == dict(west=3, east=4, south=8, north=9, **expected) + + def test_bbox_filter_nsew(s2cube): + # TODO: remove this test for deprecated `bbox_filter` im = s2cube.bbox_filter( - west=652000, east=672000, north=5161000, south=5181000, crs="EPSG:32632" + west=652000, east=672000, north=5161000, south=5181000, crs=32632 ) graph = _get_leaf_node(im) assert graph["process_id"] == "filter_bbox" assert graph["arguments"]["extent"] == { - "west": 652000, "east": 672000, "north": 5161000, "south": 5181000, "crs": "EPSG:32632" + "west": 652000, "east": 672000, "north": 5161000, "south": 5181000, "crs": 32632 } def test_bbox_filter_tblr(s2cube): + # TODO: remove this test for deprecated `bbox_filter` im = s2cube.bbox_filter( - left=652000, right=672000, top=5161000, bottom=5181000, srs="EPSG:32632" + left=652000, right=672000, top=5161000, bottom=5181000, srs=32632 ) graph = _get_leaf_node(im) assert graph["process_id"] == "filter_bbox" assert graph["arguments"]["extent"] == { - "west": 652000, "east": 672000, "north": 5161000, "south": 5181000, "crs": "EPSG:32632" + "west": 652000, "east": 672000, "north": 5161000, "south": 5181000, "crs": 32632 } def test_bbox_filter_nsew_zero(s2cube): + # TODO: remove this test for deprecated `bbox_filter` im = s2cube.bbox_filter( - west=0, east=0, north=0, south=0, crs="EPSG:32632" + west=0, east=0, north=0, south=0, crs=32632 ) graph = _get_leaf_node(im) assert graph["process_id"] == "filter_bbox" assert graph["arguments"]["extent"] == { - "west": 0, "east": 0, "north": 0, "south": 0, "crs": "EPSG:32632" + "west": 0, "east": 0, "north": 0, "south": 0, "crs": 32632 } @@ -386,7 +421,7 @@ def test_apply_absolute_str(s2cube, api_version): def test_subtract_dates_ep3129(s2cube, api_version): """EP-3129: band math between cubes of different time stamps is not supported (yet?)""" - bbox = {"west": 5.16, "south": 51.23, "east": 5.18, "north": 51.25, "crs": "EPSG:4326"} + bbox = {"west": 5.16, "south": 51.23, "east": 5.18, "north": 51.25} date1 = "2018-08-01" date2 = "2019-10-28" im1 = s2cube.filter_temporal(date1, date1).filter_bbox(**bbox).band('B04') diff --git a/tests/rest/datacube/test_zonal_stats.py b/tests/rest/datacube/test_zonal_stats.py index cdc255d07..799a56f87 100644 --- a/tests/rest/datacube/test_zonal_stats.py +++ b/tests/rest/datacube/test_zonal_stats.py @@ -13,7 +13,7 @@ def test_polygon_timeseries_polygon(connection, api_version): res = ( connection .load_collection("S2") - .filter_bbox(3, 6, 52, 50, "EPSG:4326") + .filter_bbox(3, 6, 52, 50) .polygonal_mean_timeseries(polygon) ) assert get_execute_graph(res) == load_json_resource('data/%s/aggregate_zonal_polygon.json' % api_version) @@ -26,7 +26,7 @@ def test_aggregate_spatial(connection, api_version, reducer): polygon = shapely.geometry.shape(load_json_resource("data/polygon.json")) res = ( connection.load_collection("S2") - .filter_bbox(3, 6, 52, 50, "EPSG:4326") + .filter_bbox(3, 6, 52, 50) .aggregate_spatial(geometries=polygon, reducer=reducer) ) assert get_execute_graph(res) == load_json_resource('data/%s/aggregate_zonal_polygon.json' % api_version) @@ -35,7 +35,7 @@ def test_aggregate_spatial(connection, api_version, reducer): def test_polygon_timeseries_path(connection, api_version): res = ( connection.load_collection('S2') - .bbox_filter(west=3, east=6, north=52, south=50, crs='EPSG:4326') + .filter_bbox(west=3, east=6, north=52, south=50) .polygonal_mean_timeseries(polygon="/some/path/to/GeometryCollection.geojson") ) assert get_execute_graph(res) == load_json_resource('data/%s/aggregate_zonal_path.json' % api_version) @@ -47,7 +47,7 @@ def test_aggregate_spatial_read_vector(connection, api_version, reducer): pytest.skip() res = ( connection.load_collection("S2") - .filter_bbox(3, 6, 52, 50, "EPSG:4326") + .filter_bbox(3, 6, 52, 50) .aggregate_spatial(geometries="/some/path/to/GeometryCollection.geojson", reducer=reducer) ) assert get_execute_graph(res) == load_json_resource('data/%s/aggregate_zonal_path.json' % api_version) @@ -59,7 +59,7 @@ def test_aggregate_spatial_parameter_polygon(connection, api_version): geometries = Parameter("polygon") res = ( connection.load_collection("S2") - .filter_bbox(3, 6, 52, 50, "EPSG:4326") + .filter_bbox(3, 6, 52, 50) .aggregate_spatial(geometries=geometries, reducer="mean") ) assert get_execute_graph(res) == load_json_resource('data/%s/aggregate_zonal_parameter.json' % api_version)