-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Center the coordinates to pixels for rasterio backend #1468
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
Conversation
Rasterio uses edge-based coordinates, which contradict the treatment of coordinates in xarray as being centered over the pixels. This centers them with an offset of half the resolution.
Looks good to me, and very important (for those of us who care about half a pixel! :-)... |
Thanks for the PR. I'll let @fmaussion comment on the substance here but given the confusion and the change in behavior, let's add a note in |
Thanks for the feedback @jhamman - just added the documentation. |
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.
Ups yes, I took this code from salem, but I forgot that I indeed used a corner grid back then.
doc/whats-new.rst
Outdated
@@ -21,6 +21,10 @@ v0.9.7 (unreleased) | |||
Enhancements | |||
~~~~~~~~~~~~ | |||
|
|||
- :py:func:`~xarray.open_rasterio` method now shifts the rasterio | |||
coordinates so that they are centered in each pixel. | |||
By `Greg Brener <https://github.com/gbrener>`_. |
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 change should rather appear in "Bug fixes" I think
file's geoinformation. | ||
file's geoinformation, shifted to the center of each pixel (see | ||
`"PixelIsArea" Raster Space | ||
<http://web.archive.org/web/20160326194152/http://remotesensing.org/geotiff/spec/geotiff2.5.html#2.5.2>`_ |
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 for looking into this. Isn't there a better ref than on the web archive?
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.
Happy to help @fmaussion . The web archive link is used by the GeoTIFF website (first link here: http://trac.osgeo.org/geotiff). A quick Google search didn't yield anything better.
xarray/backends/rasterio_.py
Outdated
coords['y'] = np.linspace(start=y0, num=ny, stop=(y0 + (ny - 1) * dy)) | ||
coords['x'] = np.linspace(start=x0, num=nx, stop=(x0 + (nx - 1) * dx)) | ||
coords['y'] = np.linspace(start=y0 + dy/2, | ||
num=ny, |
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.
Nit: unnecessary linebreak
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 was just to satisfy flake8
's line-length requirement. The three lines were for consistency, but I changed it (in my recent commit) to be two lines.
xarray/backends/rasterio_.py
Outdated
num=ny, | ||
stop=(y0 + (ny - 1) * dy) + dy/2) | ||
coords['x'] = np.linspace(start=x0 + dx/2, | ||
num=nx, |
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.
Nit: unnecessary linebreak
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 was just to satisfy flake8
's line-length requirement. The three lines were for consistency, but I changed it (in my recent commit) to be two lines.
Definitely a bad bug which could justify a minor release soon :-( |
Thanks for the feedback @fmaussion . I addressed your suggestions and made some small cleanups. Please let me know if there's anything else. |
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.
LGTM. I'd like to wait for @shoyer 's approval though
I'm not sure we really have a cell centering convention for xarray, except for plotting routines, where we chose the cell-centered convention from CF-conventions. But this does seem like a marginal improvement, so +1 from me. In the future we may want to switch to describing bounds fully using |
+1 from me as well. |
Thanks! |
Rasterio uses edge-based coordinates, which is a different convention from how xarray treats coordinates (based on description here: http://xarray.pydata.org/en/stable/plotting.html#coordinates). This PR centers them, offsetting by half of the resolution.
git diff upstream/master | flake8 --diff
- [ ] Closes #xxxxwhats-new.rst
for all changes andapi.rst
for new APICCing @fmaussion since he may be interested.