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

Spatila agg xvec #217

Merged
merged 9 commits into from
Feb 12, 2024
Merged

Spatila agg xvec #217

merged 9 commits into from
Feb 12, 2024

Conversation

masawdah
Copy link
Member

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.

Copy link

codecov bot commented Jan 18, 2024

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 ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ValentinaHutter
Copy link
Collaborator

ValentinaHutter commented Jan 22, 2024

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
Copy link
Collaborator

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: #124 (comment) ) 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.

@ValentinaHutter ValentinaHutter merged commit ee54156 into Open-EO:main Feb 12, 2024
5 checks passed
@clausmichele
Copy link
Member

@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:
Open-EO/openeo-processes#450

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