Skip to content
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

rename id in connection.load_collection #245

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ Minor release to address version packaging issue.

### Deprecated

- The parameter `collection_id` in `connection.load_collection()`, `datacube.load_collection()` and `imagecollectionclient.load_collection()` in favor of parameter `id`.
- The method `remove_service` in `Connection` has been deprecated in favor of `delete_service` in the `Service` class


Expand Down
14 changes: 10 additions & 4 deletions openeo/rest/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -823,32 +823,38 @@ def datacube_from_json(self, src: Union[str, Path], parameters: dict = None) ->

def load_collection(
self,
collection_id: str,
id: str = None,
spatial_extent: Optional[Dict[str, float]] = None,
temporal_extent: Optional[List[Union[str, datetime.datetime, datetime.date]]] = None,
bands: Optional[List[str]] = None,
properties: Optional[Dict[str, Union[str, PGNode, Callable]]] = None,
fetch_metadata=True,
**kwargs
) -> DataCube:
"""
Load a DataCube by collection id.

:param collection_id: image collection identifier
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be deprecated instead of being removed?

Copy link
Member Author

@jonathom jonathom Oct 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would I just put @deprecated("Use 'id' instead") above the :param collection_id line?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me, but @soxofaan should have the final vote.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can make a breaking change like this.
Existing code that uses keyword argument load_collection(collection_id="S2") will break.

There should be a fallback mechanism:

  • give id default value None
  • add a **kwargs and check if collection_id is set (and raise deprecation warning if that is the case)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I meant to say :-D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@soxofaan

def load_collection(
    self,
    id: str = None,
[...]
    if "collection_id" in kwargs:
        id = kwargs["collection_id"]
        warnings.warn("The use of `collection_id` is deprecated, use `id` instead.", DeprecationWarning)

like so?

:param id: image collection identifier
jonathom marked this conversation as resolved.
Show resolved Hide resolved
:param spatial_extent: limit data to specified bounding box or polygons
:param temporal_extent: limit data to specified temporal interval
:param bands: only add the specified bands
:param properties: limit data by metadata property predicates
:return: a datacube containing the requested data
"""

if "collection_id" in kwargs:
id = kwargs["collection_id"]
warnings.warn("The use of `collection_id` is deprecated, use `id` instead.", DeprecationWarning)

if self._api_version.at_least("1.0.0"):
return DataCube.load_collection(
collection_id=collection_id, connection=self,
collection_id=id, connection=self,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the load_collection methods of DataCube and ImageCollectionClient should also be addressed

but here it's fine to do it in a backwards incompatible way because these are practically "internal" methods nobody should be using

spatial_extent=spatial_extent, temporal_extent=temporal_extent, bands=bands, properties=properties,
fetch_metadata=fetch_metadata,
)
else:
return ImageCollectionClient.load_collection(
collection_id=collection_id, session=self,
collection_id=id, session=self,
spatial_extent=spatial_extent, temporal_extent=temporal_extent, bands=bands
)

Expand Down
18 changes: 12 additions & 6 deletions openeo/rest/datacube.py
Original file line number Diff line number Diff line change
Expand Up @@ -152,36 +152,42 @@ def process_with_node(self, pg: PGNode, metadata: Optional[CollectionMetadata] =
@classmethod
def load_collection(
cls,
collection_id: str,
id: str = None,
connection: 'openeo.Connection' = None,
spatial_extent: Optional[Dict[str, float]] = None,
temporal_extent: Optional[List[Union[str, datetime.datetime, datetime.date,PGNode]]] = None,
bands: Optional[List[str]] = None,
fetch_metadata = True,
properties: Optional[Dict[str, Union[str, PGNode, typing.Callable]]] = None
properties: Optional[Dict[str, Union[str, PGNode, typing.Callable]]] = None,
**kwargs
) -> 'DataCube':
"""
Create a new Raster Data cube.

:param collection_id: image collection identifier
:param id: image collection identifier
:param connection: The connection to use to connect with the backend.
:param spatial_extent: limit data to specified bounding box or polygons
:param temporal_extent: limit data to specified temporal interval
:param bands: only add the specified bands
:param properties: limit data by metadata property predicates
:return: new DataCube containing the collection
"""

if "collection_id" in kwargs:
id = kwargs["collection_id"]
warnings.warn("The use of `collection_id` is deprecated, use `id` instead.", DeprecationWarning)

if temporal_extent:
temporal_extent = cls._get_temporal_extent(extent=temporal_extent)
arguments = {
'id': collection_id,
'id': id,
# TODO: spatial_extent could also be a "geojson" subtype object, so we might want to allow (and convert) shapely shapes as well here.
'spatial_extent': spatial_extent,
'temporal_extent': temporal_extent,
}
if isinstance(collection_id, Parameter):
if isinstance(id, Parameter):
fetch_metadata = False
metadata = connection.collection_metadata(collection_id) if fetch_metadata else None
metadata = connection.collection_metadata(id) if fetch_metadata else None
if bands:
if isinstance(bands, str):
bands = [bands]
Expand Down
15 changes: 10 additions & 5 deletions openeo/rest/imagecollectionclient.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,32 +48,37 @@ def connection(self):

@classmethod
def load_collection(
cls, collection_id: str, session: 'Connection' = None,
cls, id: str = None, session: 'Connection' = None,
spatial_extent: Union[Dict[str, float], None] = None,
temporal_extent: Union[List[Union[str,datetime.datetime,datetime.date]], None] = None,
bands: Union[List[str], None] = None,
fetch_metadata=True
fetch_metadata=True,
**kwargs
):
"""
Create a new Image Collection/Raster Data cube.

:param collection_id: A collection id, should exist in the backend.
:param id: A collection id, should exist in the backend.
:param session: The session to use to connect with the backend.
:param spatial_extent: limit data to specified bounding box or polygons
:param temporal_extent: limit data to specified temporal interval
:param bands: only add the specified bands
:return:
"""

if "collection_id" in kwargs:
id = kwargs["collection_id"]

# TODO: rename function to load_collection for better similarity with corresponding process id?
builder = GraphBuilder()
process_id = 'load_collection'
normalized_temporal_extent = list(get_temporal_extent(extent=temporal_extent)) if temporal_extent is not None else None
arguments = {
'id': collection_id,
'id': id,
'spatial_extent': spatial_extent,
'temporal_extent': normalized_temporal_extent,
}
metadata = session.collection_metadata(collection_id) if fetch_metadata else None
metadata = session.collection_metadata(id) if fetch_metadata else None
if bands:
if isinstance(bands, str):
bands = [bands]
Expand Down
10 changes: 10 additions & 0 deletions tests/rest/test_connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -1324,6 +1324,16 @@ def test_load_collection_arguments_100(requests_mock):
}


def test_load_collection_param(requests_mock):
requests_mock.get(API_URL, json={"api_version": "1.0.0"})
conn = Connection(API_URL)
requests_mock.get(API_URL + "collections/FOO", json={})
im1 = conn.load_collection(collection_id = "FOO")
im2 = conn.load_collection(id = "FOO")
assert im1._pg.arguments["id"] == "FOO"
assert im2._pg.arguments["id"] == "FOO"


def test_load_result(requests_mock):
requests_mock.get(API_URL, json={"api_version": "1.0.0"})
con = Connection(API_URL)
Expand Down