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

filter_spatial: Clarify masking #470

Merged
merged 6 commits into from
Oct 27, 2023
Merged

filter_spatial: Clarify masking #470

merged 6 commits into from
Oct 27, 2023

Conversation

m-mohr
Copy link
Member

@m-mohr m-mohr commented Oct 12, 2023

Fixes #469

@m-mohr m-mohr added this to the 2.0.0 milestone Oct 12, 2023
Copy link
Member

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

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

some minor suggestions

CHANGELOG.md Outdated Show resolved Hide resolved
filter_spatial.json Outdated Show resolved Hide resolved
filter_spatial.json Outdated Show resolved Hide resolved
@m-mohr m-mohr requested a review from soxofaan October 13, 2023 09:59
@m-mohr
Copy link
Member Author

m-mohr commented Oct 13, 2023

One related question that came up: How is filter_bbox handled if the CRS of the bbox is different from the data cube CRS? Do you reproject the bbox to the data cube CRS first to align? Otherwise, you may also get an outside area that you may need to null. I think we should clarify that, too. What do the implementation do? @clausmichele @soxofaan @dthiex

@soxofaan
Copy link
Member

soxofaan commented Oct 13, 2023

How is filter_bbox handled if the CRS of the bbox is different from the data cube CRS? Do you reproject the bbox to the data cube CRS first to align? Otherwise, you may also get an outside area that you may need to null.

Yes we reproject (e.g. typically from lon-lat to UTM) and I should double check the implementation details, but I think the intent is to use the tightest bbox in target CRS (e.g. UTM) that covers the specified bbox (e.g. in lon-lat).
So indeed, you might get more pixels outside of the original bbox, but I don't think that is a problem in most use cases. And if it is a problem you should use explicit masking (e.g. with mask_polygon or filter_spatial) anyway.

@clausmichele
Copy link
Member

…CRS of the spatial data cube dimensions if required.
@m-mohr
Copy link
Member Author

m-mohr commented Oct 13, 2023

Thanks, I've added a clarifying remark to filter_bbox in this PR.

…CRS of the spatial data cube dimensions if required.
Copy link
Member

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

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

Fine for me as-is

@m-mohr m-mohr linked an issue Oct 26, 2023 that may be closed by this pull request
@m-mohr m-mohr merged commit f303adf into draft Oct 27, 2023
2 checks passed
@m-mohr m-mohr deleted the filter-spatial branch October 27, 2023 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

filter_spatial: clarify behaviour
3 participants