Most of Python 3 support #113
Conversation
Run python-modernize with imports fixer
| self.assertTrue(data_allclose_or_equiv(v1.values, v2.values, | ||
| rtol=rtol, atol=atol)) | ||
| assert data_allclose_or_equiv(v1.values, v2.values, rtol=rtol, atol=atol),\ | ||
| (repr(v1.values), repr(v2.values)) |
There was a problem hiding this comment.
Changes like this are just to show the values causing a failure.
Didn't read the comment when I modified this before.
|
This looks really awesome -- I will take a more careful look tonight. It would be helpful if you could update I'm OK potentially merging this even if a few tests IO related are still failing in Python 3. It's pretty obvious that the NetCDF libraries have not been very heavily tested used with Python 3, and I probably have more context to resolve those issues than you do. |
|
OK, let's see how that goes. |
|
Looks like Pydap doesn't support Python 3 yet: pydap/pydap#7 |
|
OK, I've excluded that combination from Travis. |
| self.ds = scipy.io.netcdf.netcdf_file( | ||
| filename_or_obj, mode=mode, mmap=mmap, version=version) | ||
|
|
||
| def open_store_variable(self, var): | ||
| return xray.Variable(var.dimensions, var.data, var._attributes) | ||
| return xray.Variable(var.dimensions, _decode_string_data(var.data), \ | ||
| OrderedDict((k,_decode_string(v)) for (k,v) in iteritems(var._attributes))) |
There was a problem hiding this comment.
There should probably be a _decode_attrs helper function.
As a minor style point: I don't think the \ is necessary.
There was a problem hiding this comment.
Good point. I've called it _decode_values instead because it doesn't need to know what type of dict it's passed.
|
This looks very nice, I only have a few minor complaints which I will fix up before merging. |
| data = DecodedCFDatetimeArray(data, units, calendar) | ||
| if 'units' in attributes: | ||
| units = attributes['units'] | ||
| if isinstance(units, bytes): |
There was a problem hiding this comment.
I wonder if it would be cleaner to simply ensure that all strings in attributes are unicode in the backends instead of doing it here?
There was a problem hiding this comment.
Is there any way a bytes attribute can sneak through? I'm guessing not, since you fixed the scipy backend already.
There was a problem hiding this comment.
I have a feeling that I changed this before I worked out the issues with the Scipy backend. I'll have a go at changing it back and see what might break.
No longer needed now that the scipy backend decodes all attributes
|
Thanks, I've applied all of your suggestions. |
Most of Python 3 support. I'll work on adding the rest later.
* fix version * Update conf.py
This isn't entirely finished, but I need to stop working on it for a bit, and I think enough of it is ready to be reviewed. The core code is passing its tests; the remaining failures are all in talking to the Scipy and netCDF4 backends. I also have PRs open against Scipy (scipy/scipy#3617) and netCDF4 (Unidata/netcdf4-python#252) to fix bugs I've encountered there.
Particular issues that came up:
isinstance(x, int)doesn't reliably catch numpy integer types - see e.g. numpy.int64 is not instance of int numpy/numpy#2951. I changed several such cases toisinstance(x, (int, np.integer)).