issue_comments

Thanks!

I realised that the reason that Travis and AppVeyor weren't running properly was because there were conflicts between this PR and master. I think I've managed to rebase and push properly - but let me know if I've done anything wrong.

Ah ok - that works (not entirely sure why, but it works!)

We have a bit of a circular imports mess :). For now, just import them at the top of the to_netcdf method.

Oops yes, sorry!

I'm having a bit of an issue with changing the strings to constants (which, by the way, I completely agree is how it should be done). I've defined the constants in backends/api.py and am then trying to import them in core/dataarray.py.

I am doing the import as:

from ..backends import DATAARRAY_NAME, DATAARRAY_VARIABLE

But this gives me the following error:

../../anaconda3/lib/python3.5/site-packages/py/_path/local.py:650: in pyimport __import__(modname) xarray/__init__.py:3: in <module> from .core.extensions import (register_dataarray_accessor, xarray/core/extensions.py:3: in <module> from .dataarray import DataArray xarray/core/dataarray.py:9: in <module> from ..backends import DATAARRAY_NAME, DATAARRAY_VARIABLE xarray/backends/__init__.py:8: in <module> from .netCDF4_ import NetCDF4DataStore xarray/backends/netCDF4_.py:6: in <module> from .. import Variable E ImportError: cannot import name 'Variable'

It seems like my relative import is somehow mucking up another relative import inside the netCDF4 stuff. Do you have any ideas how I can solve this? Am I just doing my imports wrong?

I have a couple inline comments that I think you missed?

Great, thanks. I think I've done these changes (frustratingly I can't see a way to use *args/*kwargs after using the full argument list in the function definition, so I've had to write it out. Let me know if there is a better way, but I suspect there isn't).

I have a few final nits, but looking very nice!

Thanks - I've got that working with the close() method, and I did it 'properly' using the BaseDataObject class.

The only failure on here now is the same as #991 - a conda error creating one of the environments.

The easiest fix would be to add a decorator to skip tests on Windows, like the existing decorators that we use: https://github.com/pydata/xarray/blob/master/xarray/test/init.py#L73

I'm not sure off hand how to detect if we're running on Windows but it can't be hard.

To add a close method, I would copy these lines from dataset.py (or better use, move them to the common base class BaseDataObject in common.py): https://github.com/pydata/xarray/blob/master/xarray/core/dataset.py#L251-L262

Then, you just need to define _file_obj = None in DataArray.__init__ and set it when appropriate.

Also, it seems like there might be an issue with not closing the Dataset - but only on Windows (see the AppVeyor build failure at https://ci.appveyor.com/project/jhamman/xarray-injyf/build/1.0.268/job/kr8ss684ijxg55be).

Any ideas how this could be sorted without having to implement .close() for DataArray (or, alternatively, how to go about implementing that, given that I don't think a DataArray contains a reference to the Dataset that it may have come from)

Thanks for the useful comments.

I've added some bits to the Sphinx docs in the places that I think they should go - but let me know if I need to add any more.

I think I've dealt with the comments/issues now, but let me know if more needs doing.

I would add these tests to test_backends, probably a new test class. The best test is probably a round-trip test where we save and load a DataArray and verify that the result is identical to what we started with. On Sun, Aug 28, 2016 at 2:20 PM Robin Wilson notifications@github.com wrote:

Thanks @shoyer https://github.com/shoyer - I wasn't actually aware that DataArrays can have a name before they're put into a Dataset (that's one of the things I love about contributing to OSS: you always learn things about the software that you never knew before!). I'll deal with the design issues ASAP.

How would you suggest testing this? I wasn't sure where to put the tests, and what exactly to test? Presumably something about the equivalence between this and saving after manually converting to a dataset, but I wasn't really sure how best to go about it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/990#issuecomment-243000159, or mute the thread https://github.com/notifications/unsubscribe-auth/ABKS1qbgrUqpkuRNqP4BjhclA25MrRN-ks5qkfusgaJpZM4Ju3Aq .

Thanks @shoyer - I wasn't actually aware that DataArrays can have a name before they're put into a Dataset (that's one of the things I love about contributing to OSS: you always learn things about the software that you never knew before!). I'll deal with the design issues ASAP.

How would you suggest testing this? I wasn't sure where to put the tests, and what exactly to test? Presumably something about the equivalence between this and saving after manually converting to a dataset, but I wasn't really sure how best to go about it.

I'm also a little concerned about the design here, because it always saves/restores the DataArray with the name 'data', which could override the original name. Ideally this method should save/restore a DataArray as faithfully as possible.

Instead, maybe we should only override the original name if it's None, or the same name as one of it's coordinates (which would otherwise result in an invalid NetCDF file).

Then we could restore by looking for a Dataset with single data variable regardless of the name, e.g.,

if len(dataset.data_vars) != 1: raise ValueError data_array, = dataset.data_vars.values()

This is great, but it needs tests and documentation :).