Fix CRS being WKT instead of PROJ.4 #2715

djhoese · 2019-01-26T16:46:11Z

See https://github.com/mapbox/rasterio/blob/master/CHANGES.txt#L7 for the change in rasterio 1.0.14 and my conda-forge issue where I discovered this: conda-forge/gdal-feedstock#262

This was put in a bugfix (micro version number) release of rasterio which made it harder to discover. @sgillies @snowman2 I saw that you brought this up and how it affected xarray (https://rasterio.groups.io/g/dev/message/68). If this pull request is a duplicate of a fix one of you made let me know.

To xarray devs, if I need to make any other changes (whats-new?) also let me know.

Closes #xxxx
Tests added
Fully documented, including whats-new.rst for all changes and api.rst for new API

See pydata/xarray#2715

sgillies · 2019-01-26T17:30:22Z

@djhoese sorry for the inconvenience! I didn't consider the change in rasterio 1.0.14 to be an API change because the behavior of CRS.from_string(crs.to_string()) was not changed. I should have looked around for more usage like that in xarray.

sgillies · 2019-01-27T03:05:28Z

xarray/backends/rasterio_.py

@@ -282,7 +282,7 @@ def open_rasterio(filename, parse_coordinates=None, chunks=None, cache=None,
        # CRS is a dict-like object specific to rasterio
        # If CRS is not None, we convert it back to a PROJ4 string using
        # rasterio itself
-        attrs['crs'] = riods.crs.to_string()
+        attrs['crs'] = riods.crs.to_proj4()


@djhoese I recommend the following:

Suggested change

attrs['crs'] = riods.crs.to_proj4()

attrs['crs'] = riods.crs.to_dict()

It's both forward and backward compatible. As far as I can tell from scanning the xarray source, the data array's crs attribute is rarely used, except as an argument to rasterio.transform where a dict should do just fine.

@sgillies It's not just how xarray uses the crs attribute but users. The reason I made this PR was because the xarray crs attribute changed from PROJ4 to WKT.

Just curious, why does it need to be a PROJ.4 string in xarray?

For now, backwards compatibility. I don't think there is any other strong reason other than what the original developer chose (it is a fairly popular CRS format). If you think it should be WKT then that should probably go in its own issue.

The main reason was serialization to NetCDF: attributes can't be dict or objects, and strings are a perfect representation of the CRS. Any other idea welcome though.

* Force rasterio version to fix PROJ4 to WKT change See pydata/xarray#2715 * Try escaping quotes in travis CI * Try single quotes in travis reqs * Try no quotes in travis config

xarray/backends/rasterio_.py

fmaussion · 2019-01-28T19:00:17Z

can your merge from master? The failing tests should be solved by #2720

pep8speaks · 2019-01-28T19:05:21Z

Hello @djhoese! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on February 06, 2019 at 16:49 Hours UTC

djhoese · 2019-01-28T19:07:24Z

@fmaussion Done. And I merged @snowman2's suggestion and fixed the indent (I'm guessing github's editor made it difficult to see but it was off by one indentation).

fmaussion

Thanks! Let's discuss how to deal with WKT and proj4 in a separate issue

fmaussion · 2019-01-30T17:35:40Z

I'm going to merge this tomorrow unless there are further concerns

dcherian · 2019-02-05T18:16:48Z

Does this need a whats-new entry?

shoyer · 2019-02-06T16:06:58Z

Does this need a whats-new entry?

Yes, I would recommend that here -- especially for cases where we're changing behavior without a deprecation cycle (which is sometime the best path forward)

djhoese · 2019-02-06T16:17:41Z

@dcherian @shoyer This PR isn't changing any functionality. It is making the same functionality available with newer versions of rasterio.

There are discussions going on regarding changing the behavior: #2723

shoyer · 2019-02-06T16:43:39Z

This PR isn't changing any functionality. It is making the same functionality available with newer versions of rasterio.

OK, good point. I do still try to lean towards adding notes about bug fixes in "what's new" but it isn't strictly required.

See https://github.com/mapbox/rasterio/blob/master/CHANGES.txt#L7

Co-Authored-By: djhoese <[email protected]>

djhoese · 2019-02-06T16:52:06Z

@shoyer Added something to the whats-new. Let me know if anything needs changing.

shoyer · 2019-02-06T16:56:17Z

thanks David, that looks great

* master: typo in whats_new (pydata#2763) Update computation.py to use Python 3 function signatures (pydata#2756) add h5netcdf+dask tests (pydata#2737) Fix name loss when masking (pydata#2749) fix datetime_to_numeric and Variable._to_numeric (pydata#2668) Fix mypy errors (pydata#2753) enable internal plotting with cftime datetime (pydata#2665) remove references to cyordereddict (pydata#2750) BUG: Pass kwargs to the FileManager for pynio engine (pydata#2380) (pydata#2732) reintroduce pynio/rasterio/iris to py36 test env (pydata#2738) Fix CRS being WKT instead of PROJ.4 (pydata#2715) Refactor (part of) dataset.py to use explicit indexes (pydata#2696)

djhoese · 2019-03-12T17:31:50Z

@shoyer Any idea when a 0.11.4 or 0.12 will be released? I'm trying to work around some other rasterio bugs and would like to remove the restriction on the rasterio version used in my CI tests, but that requires this PR.

shoyer · 2019-03-12T18:57:44Z

Hopefully quite soon! see #2777 for current status

…

On Tue, Mar 12, 2019 at 10:31 AM David Hoese ***@***.***> wrote: @shoyer <https://github.com/shoyer> Any idea when a 0.11.4 or 0.12 will be released? I'm trying to work around some other rasterio bugs and would like to remove the restriction on the rasterio version used in my CI tests, but that requires this PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2715 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABKS1hs0lknMUSnOP9A_Qw0124V8HZllks5vV-SGgaJpZM4aUQen> .

djhoese added a commit to djhoese/satpy that referenced this pull request Jan 26, 2019

Force rasterio version to fix PROJ4 to WKT change

a0352cf

See pydata/xarray#2715

djhoese mentioned this pull request Jan 26, 2019

Force rasterio version to fix PROJ4 to WKT change pytroll/satpy#595

Merged

sgillies mentioned this pull request Jan 26, 2019

[proposal] Add CRS class similar to the CRS in rasterio Toblerity/Fiona#714

Closed

sgillies reviewed Jan 27, 2019

View reviewed changes

snowman2 reviewed Jan 27, 2019

View reviewed changes

xarray/backends/rasterio_.py Outdated Show resolved Hide resolved

fmaussion approved these changes Jan 28, 2019

View reviewed changes

djhoese mentioned this pull request Jan 28, 2019

Newest 2.4.0 build causes pyproj RuntimeError projection not named conda-forge/gdal-feedstock#262

Closed

avalentino mentioned this pull request Feb 1, 2019

test_generic_image.py failure: "projection not named" pytroll/satpy#604

Closed

djhoese and others added 4 commits February 6, 2019 10:45

Fix CRS being WKT instead of PROJ.4

9a8cf31

See https://github.com/mapbox/rasterio/blob/master/CHANGES.txt#L7

Fix rasterio usage for older rasterio without to_proj4

29e7f23

Co-Authored-By: djhoese <[email protected]>

Fix indentation on rasterio AttributeError check

769c3e9

Add CRS WKT fix to whats-new

4e827b7

djhoese force-pushed the patch-1 branch from 7ceaa1c to 4e827b7 Compare February 6, 2019 16:48

shoyer merged commit 0c73a38 into pydata:master Feb 6, 2019

djhoese deleted the patch-1 branch February 6, 2019 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CRS being WKT instead of PROJ.4 #2715

Fix CRS being WKT instead of PROJ.4 #2715

djhoese commented Jan 26, 2019

sgillies commented Jan 26, 2019 •

edited

Loading

sgillies Jan 27, 2019 •

edited

Loading

djhoese Jan 27, 2019

snowman2 Jan 27, 2019

djhoese Jan 27, 2019

fmaussion Jan 28, 2019

fmaussion commented Jan 28, 2019

pep8speaks commented Jan 28, 2019 •

edited

Loading

djhoese commented Jan 28, 2019

fmaussion left a comment

fmaussion commented Jan 30, 2019

dcherian commented Feb 5, 2019

shoyer commented Feb 6, 2019

djhoese commented Feb 6, 2019

shoyer commented Feb 6, 2019

djhoese commented Feb 6, 2019

shoyer commented Feb 6, 2019

djhoese commented Mar 12, 2019

shoyer commented Mar 12, 2019 via email

	attrs['crs'] = riods.crs.to_proj4()
	attrs['crs'] = riods.crs.to_dict()

Fix CRS being WKT instead of PROJ.4 #2715

Fix CRS being WKT instead of PROJ.4 #2715

Conversation

djhoese commented Jan 26, 2019

sgillies commented Jan 26, 2019 • edited Loading

sgillies Jan 27, 2019 • edited Loading

Choose a reason for hiding this comment

djhoese Jan 27, 2019

Choose a reason for hiding this comment

snowman2 Jan 27, 2019

Choose a reason for hiding this comment

djhoese Jan 27, 2019

Choose a reason for hiding this comment

fmaussion Jan 28, 2019

Choose a reason for hiding this comment

fmaussion commented Jan 28, 2019

pep8speaks commented Jan 28, 2019 • edited Loading

Comment last updated on February 06, 2019 at 16:49 Hours UTC

djhoese commented Jan 28, 2019

fmaussion left a comment

Choose a reason for hiding this comment

fmaussion commented Jan 30, 2019

dcherian commented Feb 5, 2019

shoyer commented Feb 6, 2019

djhoese commented Feb 6, 2019

shoyer commented Feb 6, 2019

djhoese commented Feb 6, 2019

shoyer commented Feb 6, 2019

djhoese commented Mar 12, 2019

shoyer commented Mar 12, 2019 via email

sgillies commented Jan 26, 2019 •

edited

Loading

sgillies Jan 27, 2019 •

edited

Loading

pep8speaks commented Jan 28, 2019 •

edited

Loading