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

CollectionMetadata -> CubeMetadata? #464

Closed
soxofaan opened this issue Aug 24, 2023 · 4 comments · Fixed by #541
Closed

CollectionMetadata -> CubeMetadata? #464

soxofaan opened this issue Aug 24, 2023 · 4 comments · Fixed by #541

Comments

@soxofaan
Copy link
Member

# TODO: "CollectionMetadata" is also used as "cube metadata" where the link to original collection
# might be lost (if any). Better separation between rich EO raster collection metadata and
# essential cube metadata? E.g.: also thing of vector cubes.

"CollectionMetadata" got its name when data cubes where still called "ImageCollection" (instead of "DataCube"). But now that is getting a bit confusing: in openEO, "collections" is the source data one loads with load_collection, while "cubes" are the intermediate data structure one works with. So "CollectionMetadata" should better be called "CubeMetadata".

Also, CollectionMetadata contains some specific dimension metadata parsing logic (cube:dimensions/eo:bands), which is closely tied to the STAC description of collections. I think it would be better to separate this from the essential cube metadata logic.

@soxofaan
Copy link
Member Author

I think we should change it to:

  • CubeMetadata: just the bare metadata essentials for a data cube: interface to interact with the cube dimensions and their labels (if available), maybe some resolution info too. Methods allow to construct a new CubeMetadata instance from an existing one (e.g. reduce_dimension eliminates a dimension)
  • CollectionMetadata (subclass of CubeMetadata): interface to interact with all original collection (STAC) metadata. Calling a "process" like reduce_dimension produces a CubeMetadata

By making CollectionMetadata a subclass of CubeMetadata we can stay backward compatible I think, because the interface of CollectionMetadata would not really change

@jdries
Copy link
Collaborator

jdries commented Aug 24, 2023

ok, makes sense!

@soxofaan
Copy link
Member Author

Another reason for better abstraction: load_stac should produce a data cube with metadata (#425) , but that is not the same as collection metadata.

VictorVerhaert added a commit that referenced this issue Feb 23, 2024
issue #464
moved general methods from CollectionMetadata to CubeMetadata
only collection parsing specific methods are left in CollectionMetadata
This only has a refactoring effect, no functional changes for now
@VictorVerhaert VictorVerhaert linked a pull request Feb 23, 2024 that will close this issue
@VictorVerhaert
Copy link
Contributor

def _clone_and_update(
self, metadata: dict = None, dimensions: List[Dimension] = None, **kwargs
) -> CubeMetadata: # python >= 3.11: -> Self to be more correct for subclasses
"""Create a new instance (of same class) with copied/updated fields."""
# TODO: do we want to keep the type the same or force it to be CubeMetadata?
# this method is e.g. used by reduce_dimension, which should return a CubeMetadata
# If adjusted, name should be changed to e.g. _create_updated
# Alternative is to use an optional argument to specify the class to use
cls = type(self)
if dimensions == None:
dimensions = self._dimensions
return cls(metadata=metadata or self._orig_metadata, dimensions=dimensions, **kwargs)

Currently functionality does not change due to this function.
As this function checks the type and returns an object of the same class, functions like reduce_dimension (who use this function to update a metadata object) will still return a CollectionMetadata object if that was the original object.

Changing this to return a CubeMetadata changes functionality and fails some existing tests.

An alternative is to provide an optional argument to set the expected returned class, however this feels more like a quick fix and might be confusing in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants