Skip to content
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

BUG: Fix #2864 by adding the missing vrt parameters #2865

Merged
merged 5 commits into from
Apr 11, 2019

Conversation

jmichel-otb
Copy link
Contributor

@jmichel-otb jmichel-otb commented Apr 2, 2019

@fmaussion
Copy link
Member

Should I add tests for this ?

If there is a way, it would be great yes. You can build upon (or adapt) this example here:

def test_rasterio_vrt(self):
import rasterio
# tmp_file default crs is UTM: CRS({'init': 'epsg:32618'}
with create_tmp_geotiff() as (tmp_file, expected):
with rasterio.open(tmp_file) as src:
with rasterio.vrt.WarpedVRT(src, crs='epsg:4326') as vrt:
expected_shape = (vrt.width, vrt.height)
expected_crs = vrt.crs
print(expected_crs)
expected_res = vrt.res
# Value of single pixel in center of image
lon, lat = vrt.xy(vrt.width // 2, vrt.height // 2)
expected_val = next(vrt.sample([(lon, lat)]))
with xr.open_rasterio(vrt) as da:
actual_shape = (da.sizes['x'], da.sizes['y'])
actual_crs = da.crs
print(actual_crs)
actual_res = da.res
actual_val = da.sel(dict(x=lon, y=lat),
method='nearest').data
assert actual_crs == expected_crs
assert actual_res == expected_res
assert actual_shape == expected_shape
assert expected_val.all() == actual_val.all()

cc @scottyhq who wrote this code, as I don't think any of the xarray team uses this format.

Please also add a line to whats-new.rst

@pep8speaks
Copy link

pep8speaks commented Apr 4, 2019

Hello @jmichel-otb! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-04-04 09:40:50 UTC

@jmichel-otb
Copy link
Contributor Author

@fmaussion done, let's see what CI has to say about my patches ;)

I remember reading a thread somewhere on xarray github repo discussing whether xarray should include the rasterio backend or not.

I understand that bridges between two libraries are always hard to maintain, because you need to know both products (we actually have the same kind of problem with OTB and QGis), but from a user standpoint, they need to exist somewhere. I would probably never have turned to xarray if someone with the required knowledge had not implemented the rasterio backend.

Then of course the user community should take care of maintaining those backends (this is what I am doing right now).

Bridging xarray with rasterio opens xarray to the remote sensing imagery community. And behind rasterio there is gdal, which is an awesome library with so many great capabilities (like this on-the-fly reprojection during reading I mentioned).

Copy link
Member

@fmaussion fmaussion left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks for adding a test! I'll leave it open to give some time to others if they want to have a look as well.

@scottyhq
Copy link
Contributor

scottyhq commented Apr 4, 2019

@jmichel-otb completely agree on your comments and think that the remote sensing community gains a lot by making these packages work well together (gdal, rasterio, xarray, dask, etc). As more people start using the rasterio backend with various use cases hopefully there will be more contributions such as this. Thanks!

@dcherian dcherian merged commit a4305e7 into pydata:master Apr 11, 2019
@dcherian
Copy link
Contributor

Thanks @jmichel-otb

dcherian added a commit to yohai/xarray that referenced this pull request Apr 19, 2019
* master: (29 commits)
  Handle the character array dim name  (pydata#2896)
  Partial fix for pydata#2841 to improve formatting. (pydata#2906)
  docs: Move quick overview one level up (pydata#2890)
  Manually specify chunks in open_zarr (pydata#2530)
  Minor improvement of docstring for Dataset (pydata#2904)
  Fix minor typos in docstrings (pydata#2903)
  Added docs example for `xarray.Dataset.get()` (pydata#2894)
  Bugfix for docs build instructions (pydata#2897)
  Return correct count for scalar datetime64 arrays (pydata#2892)
  Indexing with an empty array (pydata#2883)
  BUG: Fix pydata#2864 by adding the missing vrt parameters (pydata#2865)
  Reduce length of cftime resample tests (pydata#2879)
  WIP: type annotations (pydata#2877)
  decreased pytest verbosity (pydata#2881)
  Fix mypy typing error in cftime_offsets.py (pydata#2878)
  update links to https (pydata#2872)
  revert to 0.12.2 dev
  0.12.1 release
  Various fixes for explicit Dataset.indexes (pydata#2858)
  Fix minor typo in docstring (pydata#2860)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants