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
Let load_earth_relief() support all resolutions and optional subregion #542
Let load_earth_relief() support all resolutions and optional subregion #542
Changes from 4 commits
d5f8ce0
fceab54
757cadf
26125cb
c5651e1
7b2abd3
80b8350
8e066f7
4b3055c
70498b6
f50db14
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.
Sorry, one more thing. What if a user wants to get a non_tiled_resolution grid with a region? The current code doesn't seem to handle this case.
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.
The initial design was to allow users to specify a subregion for all resolutions. However, due to #524, the returned grids can't support slice operation, which makes three or more tests fail. So I have to do different ways for tiled and non-tiled resolutions, to keep all tests pass.
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.
If a user really want it, they need to call
load_earth_relief
, thengrdcut
, or using xarray slice operation.I'm also OK to add "region" to all resolutions, and declare that "load_earth_relief() grids don't support slice operation" as a bug.
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.
Right, that's tricky. But, an unsuspecting user might pass in a
region
argument for resolutions06m
and above, and it would just be silently ignored (and a full grid is returned)! We should at least raise a warning orNotImplementedError
for this case.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.
Sorry, slow reply. A workaround might be to just set the
registration and gtype
directly after callinggrdcut
, since we know it from the user input toload_earth_relief
.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.
I don't understand it. Why do you say it doesn't 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.
Sorry, it will work actually, I forgot for a second that we applied the workaround in #539.
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.
You mean applying the hack only in
load_earth_relief()
function? Sounds a good idea.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.
Implemented locally, but I'm getting this error:
This is because
grid.encoding["source"]
is removed, then the sliced new grid has the default registration and gtype.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.
RIght, let's just raise a warning here then, having layers and layers of workarounds isn't good anyway.