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
Add GeometryIndex #7
Add GeometryIndex #7
Changes from 12 commits
74ec7c9
5acb0b2
61539a5
3b7e020
2f21e56
604f5fd
5be917a
3256992
ef16849
97c90f5
7b35e6c
be351e1
6cc503d
90aaf1a
8c0f83d
dbf3c7d
531e4cf
cbcaa51
fe1dada
c539562
1a84c23
1e945b7
cd1cb4e
dcc4a32
b8fbbd0
4da5b85
f01e33a
6d770d7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
Just so we don't forget about this option as well. It is not common but when loading geoms from PostGIS, this may be useful.
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 wonder if we can somehow get
xarray
to extract the CRS from theGeometryArray
and store it on the object? That would probably have to be done in a hook that would be defined here... some sort ofarray type → hook callable
mapping where the hooks return either the fullVariable
object or adata, attrs
tuple, maybe?Also, that would require some thought into how to store the
crs
. Not sure if we could copy whateverrioxarray
/odc-geo
are doing, since those seem to have one globalcrs
perDataArray
/Dataset
?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.
Yes it would be nice to extract the CRS from the GeometryArray, but for dimension coordinates Xarray currently converts the GeometryArray into a plain numpy array, therefore losing the CRS info. We need to fix that when refactoring IndexVariable in Xarray.
With a
CRSIndex
(corteva/rioxarray#588) orRasterIndex
built from three coordinates (2D spatial + spatial_ref), having one global crs per DataArray / Dataset rioxarray may not be required anymore and the conventions regarding the coordinate names may even be relaxed.While for rasters it makes sense to have a scalar coordinate holding the CRS info to avoid duplicating it, for vector data the coordinate <-> index relationship is 1:1 so we could add the CRS info directly as attribute(s) of the vector coordinate.
It may be a good idea to add CRS info (a serializable version) as a coordinate attribute, if not present. So basic CRS info (identifier) would be accessible from the DataArray / Dataset repr, and the full CRS object would be accessible from the index object. E.g.,
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.
This will also need a change if crs becomes optional.
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.
What should we do when label CRS is not None and index CRS is None? Raise or ignore the label CRS?
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.
In geopandas we warn that the two CRS are not matching but let the operation happen. I'd do the same.
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.
+1 for being consistent with geopandas. Is it for all operations that could depend on a CRS?
(Note: it might be tricky to set the right
stacklevel
for the warnings here since the index API is called in various places of Xarray's internals, but not a big deal I think).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.
When two CRS are checked against each other like in overlay or sjoin, if one gdf has a CRS and the other does not, it always warns and executes.
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.
Hmm I wonder if we shouldn't have a special case for
.sel()
. I find it convenient to just pass one or more (list/array) shapely geometries as input labels without an explicit CRS. A warning may be annoying.This file was deleted.