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

add filter_bbox #118

Merged
merged 13 commits into from
Jun 27, 2023
Merged

add filter_bbox #118

merged 13 commits into from
Jun 27, 2023

Conversation

clausmichele
Copy link
Member

@clausmichele clausmichele commented Jun 13, 2023

Adding the filter_bbox process

@clausmichele clausmichele marked this pull request as ready for review June 13, 2023 13:16
@codecov
Copy link

codecov bot commented Jun 13, 2023

Codecov Report

Merging #118 (39d82ee) into main (1b6e369) will decrease coverage by 1.19%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##             main     #118      +/-   ##
==========================================
- Coverage   77.84%   76.65%   -1.19%     
==========================================
  Files          25       25              
  Lines        1020     1071      +51     
==========================================
+ Hits          794      821      +27     
- Misses        226      250      +24     
Impacted Files Coverage Δ
...sses_dask/process_implementations/cubes/_filter.py 59.59% <54.71%> (-7.08%) ⬇️
..._dask/process_implementations/cubes/_xr_interop.py 92.42% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@LukeWeidenwalker LukeWeidenwalker left a comment

Choose a reason for hiding this comment

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

Thanks for this change, couple of comments!

@clausmichele
Copy link
Member Author

@LukeWeidenwalker I updated the code using the .openeo accessor. However I noticed that calling data.openeo.spatial_dims throws an exception if the data has a single spatial dimension (you can get to this state using reduce_dimension(dimension='x',reducer='mean') for example). So, I updated the accessor to handle this scenario.

Additionally, there's also another edge case I'm not sure how to solve, when there's a single value on x_dim or y_dim: we need to check if this coordinate is actually inside the bbox or not. If not, should we raise an exception?

@LukeWeidenwalker
Copy link
Contributor

@clausmichele I just merged #125, please let me know if this doesn't resolve the issue that was blocking this PR!

@clausmichele
Copy link
Member Author

@LukeWeidenwalker now it should be fine and ready to be merged, thanks!
There's still an edge case which I didn't have time to decide how to handle, but the process is already functional like this.
I will try to fix that later on in another PR.

Copy link
Contributor

@LukeWeidenwalker LukeWeidenwalker left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, this lgtm now!

@LukeWeidenwalker LukeWeidenwalker merged commit b8e701f into Open-EO:main Jun 27, 2023
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