-
Notifications
You must be signed in to change notification settings - Fork 179
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 CRS to the spatial dimensions. #835
Conversation
'crs' is now a string in the spatial dimensions and attributes. Tests adjusted to suit.
spatial_dims = data_array.dims[-2:] | ||
|
||
if 'crs' in data_array[data_array.dims[-1]].attrs: | ||
if data_array[spatial_dims[0]].crs != data_array[spatial_dims[1]].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.
this can fail with IndexError
, you check that crs
is present in the attributes of the second spatial dimension, but then assume first one also has it, I would use something like
set(d.get('crs', None) for d in spatial_dims)
crs = data_array[data_array.dims[-1]].crs | ||
else: | ||
# fall back option | ||
crs = obj.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.
This can fail with AttributeError
fallback should look in data_array.attr.get('crs', None)
and only then in obj.attrs.get('crs', None)
datacube/api/core.py
Outdated
|
||
for measurement in measurements: | ||
data = data_func(measurement) | ||
attrs = measurement.dataarray_attrs() | ||
attrs['crs'] = geobox.crs | ||
attrs['crs'] = str(geobox.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.
for performance and safety reasons I would convert geobox.crs to string once, that way you also save on memory as you won;t be storing multiple identical but different strings.
Codecov Report
@@ Coverage Diff @@
## develop #835 +/- ##
===========================================
- Coverage 88.66% 88.64% -0.02%
===========================================
Files 112 112
Lines 10344 10364 +20
===========================================
+ Hits 9171 9187 +16
- Misses 1173 1177 +4
Continue to review full report at Codecov.
|
if len(crs_set) > 1: | ||
raise ValueError('Spatial dimensions have different crs.') | ||
elif len(crs_set) == 1 and None not in crs_set: | ||
crs = data_array[data_array.dims[-1]].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.
Probably cleaner if it was just
elif len(crs_set) == 1:
crs = crs_set.pop()
and then :
if crs is None:
..fallback..
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.
thanks, hopefully with this more xarray operations will preserve geo registration information
@snowman2 I think everyone would love to see various geo libraries converge to common representation of projections and the ways to attach them to raster data structures like Looks like My, limited, understanding of NetCDF CF is that all projection information is stored at the Dataset level in the attributes of the I believe the correct place to attach this information is to the X/Y coordinates themselves for a couple of reasons:
So that's what this change was about. Also with this change datacube now stores CRS in a string form rather than As far as changing from "string in an attribute named crs" to something more consistent with NetCDF CF convention, that should work for us, we should be able to make that change without major costs. So long as we can expect WKT string to be present somewhere, not too keen on supporting assembling of all possible WKT parameters stored as separate fields in the attribute dictionary as suggested by CF convention. |
I read up some more on CF model, correction to my previous post
I do think datacube code should be able to interpret that information and construct proper I also think this is a very inconvenient format to maintain at run-time, basically you can't detach individual band from the dataset and most operations will drop CRS data. Attaching CRS info to coordinates makes most sense to me, for reasons outlined in the above post. However, having methods for moving between these representations more or less seamlessly would be nice to have.
|
Part of Bug squash 2019: