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

Biopar masklayer #68

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

Biopar masklayer #68

wants to merge 7 commits into from

Conversation

Pratichhya
Copy link
Contributor

@Pratichhya Pratichhya commented Dec 2, 2024

Updated the masking function in the Biopar process graph.

However, there still seems to be unittest failure, particularly in the following files:

  • apex_algorithms/algorithm_catalog/combi_expressions/sar_water_mask.json
  • apex_algorithms/algorithm_catalog/sar_coin.json
  • apex_algorithms/algorithm_catalog/advanced_band_combination.json

@jdries, do you suggest waiting for the above files before merging this PR? So that I can pull from the main again.

Please kindly let me know if I missed any points to correct 😅

@Pratichhya Pratichhya requested a review from jdries December 2, 2024 07:24
@jdries
Copy link
Contributor

jdries commented Dec 2, 2024

For spatial filtering, is there a reason not to follow this guideline?
https://esa-apex.github.io/apex_documentation/guides/udp_writer_guide.html#spatial-filtering

@Pratichhya
Copy link
Contributor Author

Pratichhya commented Dec 2, 2024

I thought to stick to this suggestion: #37 (comment) , no other specific reason

@jdries
Copy link
Contributor

jdries commented Dec 2, 2024

That suggestion was indeed confusing, but let's use the guidelines as reference. If there are reasons to change it, we should update that first.

@@ -59,8 +59,8 @@
}
},
{
"description": "This geometries defines the bounds of the resulting image. Pixels outside the polygon are set to nodata.",
"name": "geometries",
"description": "This spatial_extent defines the bounds of the resulting image. Pixels outside the polygon are set to nodata.",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit confusing: "defines the bounds" seems to suggest that spatial_extent is a bounding box construct (like in the prototypical load_collection usage), but the type of this parameter is "geojson", so a bbox isn't even valid

},
"extent": {
"from_parameter": "temporal_extent"
"spatial_extent": {
Copy link
Contributor

Choose a reason for hiding this comment

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

the argument of filter_spatial is "geometries", not "spatial_extent"

Suggested change
"spatial_extent": {
"geometries": {

"data": {
"from_node": "filtertemporal2"
},
"spatial_extent": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"spatial_extent": {
"geometries": {

@soxofaan
Copy link
Contributor

soxofaan commented Dec 2, 2024

thought to stick to this suggestion: #37 (comment) , no other specific reason

FYI: in that comment I just wanted to mention that "polygon" is not an argument name we use in standard processes, but "geometries" is (e.g. in filter_spatial, aggregate_spatial)

About the guidelines on spatial filtering: I think there is room for clarification:

In other openEO processes, the spatial filtering is called spatial_extent,

argument namespatial_extent is only used in load_collection and load_stac, and there it accepts: bbox construct, inline geojson and vector cubes

In the use case here (openeo_udp/biopar/biopar.json), the parameter spatial_extent is defined as geojson-only (no bbox-support) and is passed to filter_spatial process (which also doesn't support bbox filtering).

Wouldn't it be better to define the spatial_extent parameter as bbox+geojson+vectorcube and pass it to load_collection (instead of filter_spatial? The guidelines also recommend this

@jdries
Copy link
Contributor

jdries commented Dec 2, 2024

Wouldn't it be better to define the spatial_extent parameter as bbox+geojson+vectorcube and pass it to load_collection (instead of filter_spatial? The guidelines also recommend this

Yes, @Pratichhya the idea is that you don't have to use filter_spatial anymore, parameter goes straight into load_collection.

I also suggest to use this convenience method to declare the parameter in Python:
https://open-eo.github.io/openeo-python-client/api.html#openeo.api.process.Parameter.spatial_extent

@Pratichhya
Copy link
Contributor Author

ahh, ok, will do that; directly load the extents in load_collection. Indeed, it is an easier solution. However, for Biopar in CDSE, it was suggested that the data be clipped to the spatial extent rather than bbox (remain the same as the VITO version), so I assumed the same might be expected here.

@jdries
Copy link
Contributor

jdries commented Dec 2, 2024

@Pratichhya that's exactly what this parameter allows: clipping to bbox, geometry, vector cube, or even nothing at all and leaving it open to further downstream processing.

@jdries
Copy link
Contributor

jdries commented Dec 2, 2024

Here's a PR for that spatial_extent confusion:
ESA-APEx/apex_documentation#66

@Pratichhya
Copy link
Contributor Author

Thank you, @jdries and @soxofaan ; I just made updates as suggested above.

@soxofaan
Copy link
Contributor

soxofaan commented Dec 5, 2024

about the failing tests: I already fixed some things on master branch, so I would merge master in this feature branch

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.

3 participants