Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Generate spatial coords with rasterix.RasterIndex#855

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
benbovy wants to merge2 commits intocorteva:master
base:master
Choose a base branch
Loading
frombenbovy:integrate-rasterix-rasterindex

Conversation

benbovy
Copy link

  • SupersedesAdd raster index #846
  • Tests added
  • Fully documented, includingdocs/history.rst for all changes anddocs/rioxarray.rst for new API

Optionally generates the spatial x/y coordinates with arasterix'sRasterIndex (opt-in).

Theexample notebook linked in#846 should work here as well.

@snowman2
Copy link
Member

snowman2 commentedApr 16, 2025
edited
Loading

  • Does the xarray pin need to be updated?
  • Mind adding test(s) ensuring this works as expected?
  • Mind adding rasterix to the latest CI tests?
  • Mind adding a raster_index section to optional dependencies?

@benbovy
Copy link
Author

Yes I will do all of that before marking this PR as ready.

Does the xarray pin need to be updated?

Likely for the tests yes. Since this is an opt-in feature Rioxarray may (should) still support Xarray versions older than the minimum version required by Rasterix.

dcherian reacted with thumbs up emoji

"the spatial coordinates with a RasterIndex"
) from err

raster_index = RasterIndex.from_transform(affine, width, height)
Copy link

@dcheriandcherianApr 16, 2025
edited
Loading

Choose a reason for hiding this comment

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

I think we should also delete the transform from the grid_mapping variable like so:

grid_mapping=ds.rio.grid_mappingobj[grid_mapping].attrs.pop("GeoTransform")

followinghttps://github.com/dcherian/rasterix/blob/main/design_notes/raster_index.md#read-time and our discussions. This will trigger some more changes (from a quick search,_cached_transform will need to pull the Affine from RasterIndex)

@dcherian
Copy link

How should we bring inxproj to handle CRS here? A followup PR?

@benbovy
Copy link
Author

How should we bring in xproj to handle CRS here? A followup PR?

Yes, a separate PR will be easier I think. Interoperability between xproj and rioxarray would likely be handledat the accessor level and might require some more refactoring.

dcherian reacted with thumbs up emoji

@@ -162,7 +178,7 @@ def _get_nonspatial_coords(
src_data_array[coord].values,
src_data_array[coord].attrs,
)
return coords
returnxarray.Coordinates(coords)


def _make_coords(
Copy link

@dcheriandcherianApr 17, 2025
edited
Loading

Choose a reason for hiding this comment

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

It'd be nice to have an accessor function to set this index even if I opened a Zarr file for example. Is there something likeds.rio.assign_coords? I didn't see anything similar in the docs. Perhaps we should haveds.rio.assign_indexes(raster:bool=True, crs: bool=True) that does both RasterIndex and CRSIndex

Copy link
Member

Choose a reason for hiding this comment

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

Do you think that would be a good fit to add to a rasterix accessor? If you do that, you just setdecode_coords=False and assign the index/coords post load.

Choose a reason for hiding this comment

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

It needs a bunch of extra info : x dim name, y dim name, transform, and so it's pretty convenient to just do all that with the.rio accessor. It seems like a prudent and convenient addition

Copy link
Member

Choose a reason for hiding this comment

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

Inside rasterix, it could look for the.rio ,.odc, or.geo accessors inside the method. It could be done similar toopen_dataset where it automatically figures out how to open files based on what's available. It also allows the user select their preferred engine. If they don't exist, then raise an error.

Choose a reason for hiding this comment

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

That is an option but to me it feels a lot cleaner to add a method under therio namespace. (and similarly under the other namespaces if they thought it was a good idea). At least for rioxarray it's just a small wrapper around an existing function, so it doesn't seem like meaningful extra maintenance burden to me.

RichardScottOZ reacted with thumbs up emoji
Copy link
Author

Choose a reason for hiding this comment

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

Inside rasterix, it could look for the .rio , .odc, or .geo accessors inside the method

Hmm this wouldn't be ideal IMHO, assuming that the goal of the rasterix (and xproj) project is to provide reusable utilities to downstream packages like rioxarray, odc-geo, etc. even though technically looking for those accessors is possible without having cyclic dependencies.

Under the same assumption, rasterix and xproj should also be flexible / not strongly opinionated about the names of the spatial coordinates and dimensions, etc.

dcherian reacted with thumbs up emoji
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@dcheriandcheriandcherian left review comments

@snowman2snowman2snowman2 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@benbovy@snowman2@dcherian

[8]ページ先頭

©2009-2025 Movatter.jp