PR #233 improve default CRS (and base/height) handling in filter_bbox

soxofaan · soxofaan · commit 1fe248f34c7b · 2021-09-15T09:40:13.000+02:00
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)

Original file line number	Diff line number	Diff line change
`@@ -17,8 +17,7 @@`
`17`	`17`	`"west": 3,`
`18`	`18`	`"east": 6,`
`19`	`19`	`"north": 52,`
`20`		`- "south": 50,`
`21`		`- "crs": "EPSG:4326"`
	`20`	`+ "south": 50`
`22`	`21`	`}`
`23`	`22`	`}`
`24`	`23`	`},`