geometry relation support for data search #1205
Conversation
- optional arg in filter creation - add support in CLI filter command - remove some superfluous respx.mock annotations see #1204
There was a problem hiding this comment.
Pull request overview
This PR adds support for specifying geometry relation types (intersects, contains, within, disjoint) when filtering data searches by geometry. Previously, only the default "intersects" relation was supported implicitly.
- Added optional
relationparameter togeometry_filter()function with validation - Added
--geom-relationCLI option to theplanet data filtercommand - Removed superfluous
@respx.mockdecorators from tests that don't require HTTP mocking
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| planet/data_filter.py | Added optional relation parameter to geometry_filter() function with validation logic to ensure only valid relation types are accepted |
| planet/cli/data.py | Added --geom-relation CLI option and removed the geom_to_filter callback function, now calling geometry_filter() directly in the filter command |
| tests/integration/test_data_cli.py | Added test for geometry relation feature and removed unnecessary @respx.mock decorators from tests that don't make HTTP requests |
Comments suppressed due to low confidence (1)
planet/data_filter.py:225
- The docstring should clarify that the relation parameter is optional and provide more context. Consider updating line 225 to: "The GeometryFilter can be used to search for items with a footprint geometry that has the specified spatial relationship (intersects, contains, within, or disjoint) with the specified geometry. The default relationship is intersects."
The GeometryFilter can be used to search for items with a footprint
geometry which intersects with the specified geometry.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| permission = data_filter.permission_filter() if permission else None | ||
| std_quality = data_filter.std_quality_filter() if std_quality else None | ||
|
|
||
| geometry = data_filter.geometry_filter(geom, geom_relation) |
There was a problem hiding this comment.
The geometry_filter function is called unconditionally, but it will fail when geom is None (which happens when --geom is not specified). The removed geom_to_filter callback previously handled this by only calling geometry_filter when geom was not None. This should be changed to: geometry = data_filter.geometry_filter(geom, geom_relation) if geom else None
| geometry = data_filter.geometry_filter(geom, geom_relation) | |
| geometry = data_filter.geometry_filter(geom, geom_relation) if geom else None |
| help="""Optional geometry search refinement, defaults to intersects. | ||
| May also be contains, within, or disjoint.""", | ||
| ) |
There was a problem hiding this comment.
The help text contains redundant information. It states "defaults to intersects" and then repeats "May also be contains, within, or disjoint" which doesn't include intersects in the list. The help text should clarify that intersects is one of the four options and is the default, for example: "Geometry search relation. Options are intersects (default), contains, within, or disjoint."
| help="""Optional geometry search refinement, defaults to intersects. | |
| May also be contains, within, or disjoint.""", | |
| ) | |
| help="""Geometry search relation. Options are intersects (default), contains, within, or disjoint.""", | |
| ) |
|
|
||
|
|
||
| def geometry_filter(geom: dict) -> dict: | ||
| def geometry_filter(geom: dict, relation: str = None) -> dict: |
There was a problem hiding this comment.
The type annotation should indicate that relation is Optional. Change relation: str = None to relation: Optional[str] = None to properly express that this parameter can be None.
| def geometry_filter(geom: dict, relation: str = None) -> dict: | |
| def geometry_filter(geom: dict, relation: Optional[str] = None) -> dict: |
| def test_data_filter_geom_relation(request, invoke): | ||
| geom = request.getfixturevalue("geom_geojson") | ||
| geom_str = json.dumps(geom) | ||
| result = invoke(["filter", f'--geom={geom_str}', '--geom-relation=disjoint']) | ||
| assert result.exit_code == 0 | ||
|
|
||
| and_filter = json.loads(result.output) | ||
| assert and_filter["config"][0]["relation"] == "disjoint" | ||
|
|
There was a problem hiding this comment.
Consider adding a test case to verify the behavior when --geom-relation is specified without --geom. This should ideally be handled gracefully (either ignored or produce a clear error message).
I considered adding this for the other CLI commands but this would have required significant reworking how geometry is handled, as we currently pass it along as a top-level item in the request and relation is only supported in a filter.
While it would be conceivable to unpack a user's filters and merge in a geometry filter, this felt beyond the scope of the issue.
In other words, with this MR, the only way to use relation via the CLI is to use
planet data filter --geom=... --geom-relation=within | planet data search psscene --filter=-Related Issue(s):
Closes #1204
Proposed Changes:
For inclusion in changelog (if applicable):
PR Checklist: