search

Open-EO / openeo-processes-dask

Python implementations of many OpenEO processes, dask-friendly by default.
Apache License 2.0
19 stars 14 forks source link

Spatila agg xvec #217

Closed masawdah closed 7 months ago

masawdah commented 7 months ago

Hi team, This pull request utilized the new functionality of XVEC for spatial aggregation , addressing all the previously mentioned issues related to dependencies and Dask-based computation.

codecov[bot] commented 7 months ago

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (3ffefe7) 80.98% compared to head (276c3af) 83.25%. Report is 11 commits behind head on main.

Files Patch % Lines
...es_dask/process_implementations/cubes/aggregate.py 92.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #217 +/- ## ========================================== + Coverage 80.98% 83.25% +2.27% ========================================== Files 29 29 Lines 1541 1529 -12 ========================================== + Hits 1248 1273 +25 + Misses 293 256 -37 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ValentinaHutter commented 7 months ago

Thank you for providing this! Nice to have it in xvec directly! I will have a look at the implementation in the next days and see if we can run this on the polygons over Europe as well! I have also been working on testing the options we have for the large European extent again and will update you on these soon! I think, your implementation in xvec is a good general solution, and I will check it for the use case and get back to you :)

ValentinaHutter commented 7 months ago

Thanks a lot for all your effort for this!

I had a closer look at the aggregate_spatial process for the geometries distributed all over Europe. (You can find information about it here: https://github.com/Open-EO/openeo-processes-dask/issues/124#issuecomment-1713574040 ) I was trying to find a general solution for the UC, but it seems like the geometries all over Europe really need to be treated specially - so I ended up with a first temporary implementation in our eodc backend. (Let me know, if you are interested in the reasons behind this - then I can describe it more in detail. Basically, the solution is very backend specific and I do not want to see it as a general solution - but I can share the code with you, or describe what I did. )

I think, it would be great to have your implementation in openeo-processes-dask, as it is much more general and should work for any backend. The only thing that would be nice to add, before we merge it though, would be a test that also checks some of the values for the geometries, since right now, the check only makes sure that the number of dimensions was reduced.

clausmichele commented 7 months ago

@ValentinaHutter probably you missed one specific bit which is back-end specific? Because aggregate_spatial does not allow to provide a url and load the geometries from it. The specific process load_url should be used instead. Maybe comment here since there is an ongoing discussion about this: https://github.com/Open-EO/openeo-processes/issues/450