Refactor the _load_remote_dataset function to load tiled and non-tiled grids in a consistent way #3120
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
seisman
force-pushed
the
datatypes/gmtgrid
branch
4 times, most recently
from
d0517e0
to
4ba67df
Compare
seisman
force-pushed
the
refactor/load_remote_dataset
branch
8 times, most recently
from
278f21e
to
d18fc1d
Compare
seisman
force-pushed
the
refactor/load_remote_dataset
branch
from
d18fc1d
to
004f985
Compare
seisman
changed the title
seisman
changed the title
seisman
added
enhancement
seisman
commented
Apr 19, 2024
Comment on lines
+430
to
+434
| # Full path to the grid if not tiled grids. | ||
| source = which(fname, download="a") if not resinfo.tiled else None | ||
| # Manually add source to xarray.DataArray encoding to make the GMT accessors work. | ||
| if source: | ||
| grid.encoding["source"] = source |
There was a problem hiding this comment.
Currently, the GMT accessors require the grid.encoding["source"] to work. So, for non-tiled gris, we get the full path to the grid and set grid.encoding["source"]. For tiled grids, grid.encoding["source"] is undefined.
This was referenced Apr 19, 2024
seisman
commented
Apr 19, 2024
| if dataset.resolutions[resolution].tiled: | ||
| raise GMTInvalidInput( | ||
| f"'region' is required for {dataset.title} resolution '{resolution}'." | ||
| fname = f"@{dataset_prefix}{resolution}_{registration[0]}" |
There was a problem hiding this comment.
I also propose to
- Rename
dataset_nametoname - Rename
dataset_prefixtoprefix - Remove the trailing
_fromdataset_prefix, e.g.,dataset_prefix="earth_relief_"should beprefix="earth_relief".
Please leave your comments before I make the changes.
There was a problem hiding this comment.
I feel it's better to do it in a separate PR to make this PR small for review.
seisman
commented
Apr 22, 2024
| f"'region' is required for {dataset.title} resolution '{resolution}'." | ||
| ) | ||
|
|
||
| # Currently, only grids are supported. Will support images in the future. |
There was a problem hiding this comment.
Please note that: currently we use -Tg so it only works for grids. For images, we need to use -Ti.
To support images, we need to know whether the remote dataset is a grid or an image. It means we need to add the data kind information to the GMTRemoteDataset class.
michaelgrund
approved these changes
Apr 22, 2024
seisman
added
final review call
seisman
removed
the
final review call
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of proposed changes
The
_load_remote_datasetfunction uses two different ways when loading tiled and non-tiled remote datasets.whichto download the grid, get the full path and load it by callingload_dataarray(i.e.,xr.load_dataarray)grdcut, which downloads the needed tiles, assembles them into a single grid and loads it by callingload_dataarray.Currently, both tiled and non-tiled grids are loaded from a grid file on disk by
xr.load_dataarray, so the returned xarray.DataArray are similar for tiled and non-tiled grids. The only difference is, the source file given ingrid.encoding["source"]doesn't exist for tiled grids, because the assembled grid is a temporary file.We're working on refactoring the
grdcutwrapper to use virtual files in #3115. After #3115 is finished, the returned xarray.DataArray would have different attributes for tiled and non-tiled grids. So, it's better to have a consistent way for loading tiled and non-tiled grids. This can be done by calling the specialreadmodule. Its syntax looks like (the specialreadmodule is only available for external wrappers, so the command doesn't work in bash):in which,
vfilecan be an actual file name or a virtual file name, and-Tgmeans the input is a grid.