- Notifications
You must be signed in to change notification settings - Fork91
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
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Create a RasterIndex if option use_raster_index is True.
snowman2 commentedApr 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
Yes I will do all of that before marking this PR as ready.
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. |
"the spatial coordinates with a RasterIndex" | ||
) from err | ||
raster_index = RasterIndex.from_transform(affine, width, height) |
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 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 commentedApr 16, 2025
How should we bring in |
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. |
@@ -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( |
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.
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
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.
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.
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.
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
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.
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.
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.
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.
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.
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.
docs/history.rst
for all changes anddocs/rioxarray.rst
for new APIOptionally generates the spatial x/y coordinates with arasterix's
RasterIndex
(opt-in).Theexample notebook linked in#846 should work here as well.