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

Issue464 cube metadata #541

Merged
merged 12 commits into from
Feb 29, 2024
Merged

Issue464 cube metadata #541

merged 12 commits into from
Feb 29, 2024

Conversation

VictorVerhaert
Copy link
Contributor

from issue #464

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 an issue Feb 23, 2024 that may be closed by this pull request
@VictorVerhaert
Copy link
Contributor Author

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.

Copy link
Member

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

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

some initial comments

openeo/metadata.py Outdated Show resolved Hide resolved
openeo/metadata.py Outdated Show resolved Hide resolved
openeo/metadata.py Outdated Show resolved Hide resolved
openeo/metadata.py Outdated Show resolved Hide resolved
@soxofaan
Copy link
Member

Currently functionality does not change due to this function.

I think it's fine to keep the original type of the metadata, that will indeed avoid breakage at other places.

Priority at the moment is having a base metadata class that is not implicitly tied to collection metadata (e.g. for #425 #516 #527 etc)

_clone_and_update had to be overridden by CollectionMetadata to keep the _orig_metadata attribute
@VictorVerhaert
Copy link
Contributor Author

_orig_metadata has been removed from CubeMetadata.
This causes CollectionMetadata to override the _clone_and_update method in order to preserve the orig_metadata (CollectionMetadata cannot be initiated without metadata, as it should be imo).

Unless we change _clone_and_update to always return a CubeMetadata this is the only clean solution I think.

CubeMetadata tests still need to be written.

Copy link
Member

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

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

some quick notes

openeo/metadata.py Outdated Show resolved Hide resolved
openeo/metadata.py Outdated Show resolved Hide resolved
== None -> is None
set_dimensions -> _set_dimensions
this was less complex and more readable than the previous implementation
@VictorVerhaert
Copy link
Contributor Author

VictorVerhaert commented Feb 27, 2024

TODO:

  • Write test cases specific for CubeMetadata
  • add type checking for different dimensions

BandsDimension and TemporalDimension needed to override rename and rename labels
to ensure that the type checks were correct
@VictorVerhaert VictorVerhaert marked this pull request as ready for review February 28, 2024 10:21
Copy link
Member

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

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

some more notes

about test_metadata.py: be careful about adding formatting changes everywhere, that makes it hard to to find the actual things that were changed

can you also add a changelog entry?

openeo/metadata.py Outdated Show resolved Hide resolved
openeo/metadata.py Outdated Show resolved Hide resolved
openeo/metadata.py Outdated Show resolved Hide resolved
openeo/metadata.py Outdated Show resolved Hide resolved
@VictorVerhaert
Copy link
Contributor Author

Because I changed the names of the metadata tests using CollectionMetadata to test_collectionmetadata, pre-commit refactored the whole tests causing a large diff.
I'll be more careful with this in the future.

I added an entry in the changelog and processed the last review.

CHANGELOG.md Outdated Show resolved Hide resolved
@soxofaan soxofaan merged commit 70b43f2 into master Feb 29, 2024
9 checks passed
@soxofaan soxofaan deleted the issue464-CubeMetadata branch February 29, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CollectionMetadata -> CubeMetadata?
2 participants