Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 new
odc.mask
andodc.crop
methods for masking and croppingxarray
data by geometries #114Add new
odc.mask
andodc.crop
methods for masking and croppingxarray
data by geometries #114Changes from 1 commit
bf2a1c9
ffd4cb3
40330a7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So,
xx.where
in that form always changes output tofloat{64|32}+nan
representation. Which is a valid and often desired output format, but not always. One might also expect to retain existing pixel data type withnodata
on output.We should certainly support both. But it's less clear to me which one should be a default
int16, nodata=-9999
on output if that was given on input, but I can see howfloat32, nan
and evenfloat64, nan
can be also desired.float32, nan
output formask
operation.In the code as it stands we should at least ensure that
nodata
attribute is not present onxx_masked
output array, I still don't have any intuition forxarray
attributes, when they stay and when they go...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this occurred to me too - I definitely think both options should be supported. My take would be that "most" of our more general users simply want a way to quickly set pixels outside of a polygon to
NaN
- which is a very common workflow for things like pixel drills or zonal statistics. Getting backint16, nodata=-9999
is definitely useful but perhaps for a smaller number of more advanced users/workflows. I would be in favour of the simplerNaN
option being the default.The approach taken in
dea_tools.datahandling.load_ard
seems like it could be a good template: https://github.com/GeoscienceAustralia/dea-notebooks/blob/develop/Tools/dea_tools/datahandling.py#L214-L221dtype="auto"
: When 'auto' is used, the data will be converted tofloat32
if masking is used, otherwise data will be returned in the native data type of the data.dtype="native"
: Data will be returned in the native data type of the data, including native nodata valuedtype='float{16|32|64}'
: Custom dtypeI guess given
mask
always applies a mask, we probably don't need "auto"... so a default of "float32" with an option for "native" could work.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the moment, I've added some code to strip nodata attributes:
https://github.com/opendatacube/odc-geo/blob/crop/odc/geo/_xr_interop.py#L291-L296
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same story as before: it's dangerous to perform coordinate transform, if it can be avoided by pushing into more basic operations like
.enclosing
, then it should be. You'd then check if overlap_roi returns a null set and raise error then.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, have revised to remove the coordinate transform, and simply run
enclosing
>overlap_roi
then verify the ROI isn't empty usingroi_is_empty
:https://github.com/opendatacube/odc-geo/blob/crop/odc/geo/_xr_interop.py#L330-L341
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where did this come from? bad merge or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think I accidently went one commit to far when running
git reset --soft HEAD~<number of commits to merge into one>
. Although it confuses me that the same code is in thecrop
branch anddevelop
but is still being detected as a diff...https://github.com/opendatacube/odc-geo/blob/crop/odc/geo/_xr_interop.py#L768-L773
https://github.com/opendatacube/odc-geo/blob/develop/odc/geo/_xr_interop.py#L660-L665