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

allow using udp parameter for spatial/temporal extents #144

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jdries
Copy link
Collaborator

@jdries jdries commented Jul 25, 2024

pull request to allow passing udp Parameter objects

@jdries jdries marked this pull request as ready for review October 9, 2024 06:56
@jdries
Copy link
Collaborator Author

jdries commented Oct 9, 2024

@HansVRP @VincentVerelst in the frame of adding worldcereal UDP's to apex catalog, can someone take over this PR and check what is still relevant and needed today?
It's not super urgent, but we probably want to move this forward.

Related to: ESA-APEx/apex_algorithms#28

Copy link
Collaborator

@HansVRP HansVRP left a comment

Choose a reason for hiding this comment

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

Main issue is that the linting is failing. the repo requires the use of pre-commit, black and isort

if (
temporal_extent is not None
temporal_extent is not None and not isinstance(temporal_extent, Parameter)
Copy link
Collaborator

Choose a reason for hiding this comment

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

for sake of readibility, would it make sense to also introdue temporal_is_param = isinstance(temporal_extent, Parameter)

): # Can be ignored for intemporal collections such as DEM
temporal_extent = [temporal_extent.start_date, temporal_extent.end_date]

if fetch_type == FetchType.TILE:
assert isinstance(
spatial_extent, BoundingBoxExtent
), "Please provide only a bounding box for tile based fetching."
spatial_extent = dict(spatial_extent)
) or spatial_is_param, "Please provide only a bounding box for tile based fetching."
Copy link
Collaborator

Choose a reason for hiding this comment

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

reading deeper into this _load collection I think it would be useful to modularize the getting the temporal and spatial extent.

I think something like:

 # Helper function for temporal extent processing
    def process_temporal_extent(temporal_extent):
        if isinstance(temporal_extent, Parameter) or temporal_extent is None:
            return temporal_extent
        return [temporal_extent.start_date, temporal_extent.end_date]

    # Helper function for spatial extent processing
    def process_spatial_extent(spatial_extent, fetch_type):
        if fetch_type == FetchType.TILE:
            if not isinstance(spatial_extent, BoundingBoxExtent) and not isinstance(spatial_extent, Parameter):
                raise ValueError("Please provide only a bounding box for tile-based fetching.")
            return spatial_extent if isinstance(spatial_extent, Parameter) else dict(spatial_extent)

        elif fetch_type == FetchType.POINT:
            if not isinstance(spatial_extent, GeoJSON) or spatial_extent.get("type") != "FeatureCollection":
                raise ValueError("Please provide a valid GeoJSON FeatureCollection for point-based fetching.")
            return spatial_extent

        elif fetch_type == FetchType.POLYGON:
            if isinstance(spatial_extent, GeoJSON):
                if spatial.get("type") != "FeatureCollection":
                    raise ValueError("Please provide a valid FeatureCollection type of GeoJSON.")
                return spatial_extent
            elif isinstance(spatial_extent, str):
                if not (spatial_extent.startswith("https://") or spatial_extent.startswith("http://")):
                    raise ValueError("Please provide a valid URL to a GeoParquet or GeoJSON file.")
                return connection.load_url(spatial_extent, format="Parquet" if ".parquet" in spatial else "GeoJSON")
            else:
                raise ValueError("Please provide a valid URL to a GeoParquet or GeoJSON file.")
        
        return spatial_extent

should work.

The only part which would not be able to be modularized is:

    if fetch_type == FetchType.POLYGON and isinstance(spatial_extent, (str, GeoJSON)):
        cube = cube.filter_spatial(spatial_extent)

thoughts?

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.

2 participants