Skip to content

Store first tile as source encoding for tiled grids #3950

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

Merged
merged 7 commits into from
May 14, 2025
Merged

Conversation

weiji14
Copy link
Member

@weiji14 weiji14 commented May 14, 2025

Description of proposed changes

Tiled remote dataset grids used to have an undefined grid.encoding["source"] because there was a list of grids returned from pygmt.which, and we did not select any one. The change here means that we set grid.encoding["source"] to the first grid in the list (sorted alphabetically).

This means that slicing xarray.DataArray tiled grids should return proper GMT Accessor GridType and GridRegistration values, because the source file can be queried using grdinfo to get the values.

Addresses #3932 (comment) and #524

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If wrapping a new module, open a 'Wrap new GMT module' issue and submit reasonably-sized PRs.
  • If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash command is:

  • /format: automatically format and lint the code

weiji14 added 3 commits May 14, 2025 15:49
So that GMT accessor info works with tiled grids too. Adapted from #3932.
Slicing a tiled grid retains the original source still, but doing math operations like addition cause source to be lost and fallback to default registration/gtype.
Resolves inconsistency between CI and local seen at 11436ad
@weiji14 weiji14 added this to the 0.16.0 milestone May 14, 2025
@weiji14 weiji14 self-assigned this May 14, 2025
@weiji14 weiji14 added the enhancement Improving an existing feature label May 14, 2025
Comment on lines 595 to 601
# Full path to the grid
source: str | list = which(fname, download="a")
if resinfo.tiled:
source = sorted(source)[0] # get first grid for tiled grids
# Manually add source to xarray.DataArray encoding to make the GMT accessors work.
if source:
grid.encoding["source"] = source
Copy link
Member Author

Choose a reason for hiding this comment

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

Could add verbose="q" to the which call here, and probably remove the if source: line, but doesn't really matter if it's all gonna go away after the refactor to use xr.load_dataarray in #3932.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer to have verbose="d" added and if source removed, so that this PR is fully functional on its own.

@weiji14 weiji14 marked this pull request as ready for review May 14, 2025 05:30
Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

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

Looks good except one minor edit.

@seisman seisman merged commit 12f9776 into main May 14, 2025
21 of 24 checks passed
@seisman seisman deleted the tiled_source_first branch May 14, 2025 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants