issue_comments

Huh, I would not have expected that behavior. To my mind, it seems unwanted, but then again I don't totally follow the nuances of #2180. Thanks for the heads up.

Ah, I see, merge instead of rebase. I'll try that next time.

Ugh, sorry about that. I'm following the instructions in contributing to xarray. It's a fetch/rebase. Happy to do it another way if there is a better practice. Thanks.

Updated the try/except following @keewis's lead. Also sparse 0.11.0 has been released so I was able to check that the warning is correctly issued for 0.10.0, then I upgraded, and the warning is no longer issued (and sparse doesn't choke on the casting kwarg).

Thanks for the review @dcherian. I modified the tests as suggested and added some docstring updates. Let me know what you think.

Did a fetch and rebase, not sure why commits from other PRs added to master in the meantime show up in this one 🤷

I think(?) the pep warnings are from the commits @max-sixty pushed several days ago during testing

Maybe I am overcomplicating things but I came up with this solution:

Catch the error. If it is the unallowed casting kwarg error, and we are dealing with a sparse array, pop the casting kwarg and issue a RuntimeWarning that we are doing so. Otherwise, raise the original error since it might be something else non-sparse related.

Does that seem reasonable?

My solution of try/except and popping casting from kwargs felt a bit kludgey, so I'm definitely open to other ideas. Letting it raise and xfailing the test would mean that we can no longer perform astype on sparse arrays until a new sparse version is released in the future. Though I suppose that would depends on the next xarray vs sparse release timeline.

Nice! Thanks a million @dcherian for raising that issue over at sparse.

@max-sixty regarding the minimum version, it seems like sparse has a relatively infrequent release cycle (many months) so it could be a while before we have a minimum version to require. Could we check whether we are dealing with a sparse array and call the numpy method instead for that use case, until sparse supports it?

Thanks @max-sixty. I pushed a second commit, including adding a keep_attrs argument so people can keep the old behavior of losing the attrs. I added a bit more testing on for general astype behavior, checking dtypes are correct and values are similar.

What do you make of the failing tests in test_sparse.py (test_dataarray_method and test_variable_method)? I'm not really familiar enough with the pytest parametrize syntax to figure out what's going on.

Just ran into this issue.

If it's really just a matter of expanding tests and resolving conflicts of @gajomi's PR to the current codebase I can take this on, but I don't want to take their credit. Is there an easy way for a non-owner to modify a PR or should I submit a new one?

Thanks for all your work @dcherian! Did a quick test with some real-world timeseries data I've been wanting to use with max_gap and it looks good to me. I will definitely be using this in the future! 👍

The NC_STRING type enables arrays of strings to be serialized as attributes in nc files. NC_STRING is not in netcdf3, only netcdf4, so @jhamman is right. xarray has supported NC_STRING in conjunction with netcdf4 since 0.10.4. Not sure how it wasn't throwing an error for @jsadler2 on 0.12.1 sans netcdf4 🤷‍♂.

Thanks so much for taking this on. Not sure how I missed the notification that you had done so.

@dcherian I checked out your branch and did some testing and found that testing this against dim="x" fails with a ValueError. ```python arr = np.array([ [np.nan, np.nan, np.nan, 1, 2, 3, 4, np.nan, 6, 7, np.nan, np.nan, np.nan, 11, np.nan, np.nan, ], [np.nan, np.nan, np.nan, 1, 2, 3, np.nan, np.nan, 6, 7, np.nan, np.nan, np.nan, 11, np.nan, np.nan, ], [np.nan, np.nan, np.nan, 1, 2, 3, np.nan, np.nan, 6, 7, np.nan, np.nan, np.nan, 11, np.nan, np.nan, ], [np.nan, np.nan, np.nan, 1, 2, 3, 4, np.nan, 6, 7, np.nan, np.nan, np.nan, 11, np.nan, np.nan, ], ])

da = xr.DataArray(arr, dims=["x", "y"], coords={"x": np.arange(arr.shape[0]), "y": np.arange(arr.shape[1])})

actual = da.interpolate_na("y", max_gap=2) # this works as in your test example actual = da.interpolate_na("x", max_gap=2) # this fails Error:

actual = da.interpolate_na("x", max_gap=2) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/Users/dnowacki/Documents/xarray/xarray/core/dataarray.py", line 2026, in interpolate_na **kwargs File "/Users/dnowacki/Documents/xarray/xarray/core/missing.py", line 302, in interp_na nan_block_lengths = _get_nan_block_lengths(self, dim, index) File "/Users/dnowacki/Documents/xarray/xarray/core/missing.py", line 23, in _get_nan_block_lengths arange = ones_like(obj) * index + (index[1] - index[0]) File "/Users/dnowacki/Documents/xarray/xarray/core/dataarray.py", line 2500, in func if not reflexive File "/Users/dnowacki/Documents/xarray/xarray/core/variable.py", line 1884, in func if not reflexive ValueError: operands could not be broadcast together with shapes (4,16) (4,) ```

I fixed it by changing this line https://github.com/dcherian/xarray/blob/8899f05a13533e10c8972836afda089b5b792ca0/xarray/core/missing.py#L23 to this: python if index.shape[0] == obj.shape[0]: arange = ones_like(obj) * index[:, None] + (index[1] - index[0]) else: arange = ones_like(obj) * index + (index[1] - index[0]) Does this seem reasonable?

Thanks @dcherian. I think this one's ready to go.

Sounds good. This latest commit un-deprecates tutorial.load_dataset() and makes it parallel to xarray.load_dataset() in terms of functionality.

Got it, thanks. I can make this change, assuming the dask argument used for deprecating it in the first place isn't stronger than maintaining this consistency.

Forgive me, I don't understand what you mean by download vs open. I see that tutorial.load_dataset() is different from this PR in that it keeps the file handle open instead of closing it. Is that the distinction you are referring to? Otherwise all the gallery/examples I have checked call tutorial.load_dataset() at the beginning and that's it. Unless I'm missing something?

https://github.com/pydata/xarray/blob/6d93a95d05bdbfc33fff24064f67d29dd891ab58/xarray/tutorial.py#L92-L107

Seems like undeprecating it makes sense as a function with identical functionality is being added here. What if we just imported the new load_dataset as xarray.tutorial.load_dataset for the purposes of the tutorial?

Took a stab at implementing these functions.

Good to go as far as I'm concerned. I'm not even sure how I'd phrase these changes in whats-new, so let's go with it as-is.

Thanks @shoyer. I think numeric indexes are clearer, though maybe not cleaner. Removed them and fixed the dask CI test I didn't catch the first time through. Hopefully everything passes now. 🤞

I used to use the autoclose=True argument to open_dataset() all the time to avoid this issue. Then it was deprecated with the LRU introduction, and I now do a two-line ds = open_dataset().load() and then ds.close(). Just adding my use-case and what I found to be a regression in terms of usability. Something like load_dataset() would be helpful but I wonder if adding an argument to the existing open_dataset() would be better.

Yep that does it, thanks! :thumbsup:

I guess I could have read the "sequence of str" description in the docs more closely. Maybe it would make sense to accept a single string in addition to a sequence of strings?

@shoyer Thank you for the helpful feedback. I think I addressed all of your comments, and also expanded the tests.

I only have a vague grasp of proper object oriented programming practices; not sure if the helper functions should be within the class or what, but it does seem to work this way.

OK, after some false starts, I think this is working, but the CI build is failing on CONDA_ENV=py35 and py36... but it's failing on a different test (specific to netcdf3, complaining about a temp file) and I can't see how it is impacted by my changes... thoughts?

Ah, nice, thanks. Glad to see it will be default behavior in the future. For kicks, I decided to try to implement it on my own. Pretty much stumbling around in the dark here, but I think I got it working. PR forthcoming.

@jhamman In brief, it's weird.

Engine | encoding['_FillValue'] = False | Do nothing ----- | ----- | ----- netCDF4 | Filling off | Filling on scipy | Filling off | Filling off h5netcdf | Filling on | Filling off

So, this is some peculiar behavior. Setting _FillValue to False works for netCDF4 (as we have seen), has no effect using the scipy engine, and seems to do the opposite of intended for h5netcdf.

Code below:

``` import xarray as xr import numpy as np import pandas as pd

ds = xr.Dataset({'foo': (('x', 'y'), np.random.rand(4, 5))}, coords={'x': [10, 20, 30, 40], 'y': pd.date_range('2000-01-01', periods=5), 'z': ('x', list('abcd'))})

ds.to_netcdf('notset_scipy.nc', engine='scipy') ds.to_netcdf('notset_netcdf4.nc', engine='netcdf4') ds.to_netcdf('notset_h5netcdf.nc', engine='h5netcdf')

ds.y.encoding['_FillValue'] = False

ds.to_netcdf('False_scipy.nc', engine='scipy') ds.to_netcdf('False_netcdf4.nc', engine='netcdf4') ds.to_netcdf('False_h5netcdf.nc', engine='h5netcdf') ```

netCDF4 $ ncinfo -v y notset_netcdf4.nc <type 'netCDF4._netCDF4.Variable'> int64 y(y) units: days since 2000-01-01 00:00:00 calendar: proleptic_gregorian unlimited dimensions: current shape = (5,) filling on, default _FillValue of -9223372036854775806 used $ ncinfo -v y False_netcdf4.nc <type 'netCDF4._netCDF4.Variable'> int64 y(y) units: days since 2000-01-01 00:00:00 calendar: proleptic_gregorian unlimited dimensions: current shape = (5,) filling off

scipy $ ncinfo -v y notset_scipy.nc <type 'netCDF4._netCDF4.Variable'> int32 y(y) units: days since 2000-01-01 00:00:00 calendar: proleptic_gregorian unlimited dimensions: current shape = (5,) filling off

$ ncinfo -v y False_scipy.nc <type 'netCDF4._netCDF4.Variable'> int32 y(y) units: days since 2000-01-01 00:00:00 calendar: proleptic_gregorian _FillValue: 0 unlimited dimensions: current shape = (5,) filling off

h5netcdf $ ncinfo -v y notset_h5netcdf.nc <type 'netCDF4._netCDF4.Variable'> int64 y(y) units: days since 2000-01-01 00:00:00 calendar: proleptic_gregorian unlimited dimensions: current shape = (5,) filling off

$ ncinfo -v y False_h5netcdf.nc <type 'netCDF4._netCDF4.Variable'> int64 y(y) _FillValue: 0 units: days since 2000-01-01 00:00:00 calendar: proleptic_gregorian unlimited dimensions: current shape = (5,) filling on

Allowing {'_FillValue': False} to indicate that _FillValue should not be included would be a simple, easy fix, so we should probably do that regardless.

Correct me if you're talking about something different, but xarray already supports setting _FillValue to False to turn off filling. (Is there any use case where filling remains on but without a valid _FillValue?) For example, I have a netCDF processing routine using xarray. In the code I have the line for the lon dimension:

ds.lon.encoding['_FillValue'] = False

Which, for the relevant dimension, yields in ncinfo:

$ ncinfo -v lon yesfalse.nc <type 'netCDF4._netCDF4.Variable'> float64 lon(lon) units: degrees_east long_name: Longitude epic_code: 502 unlimited dimensions: current shape = (1,) filling off

If I comment out that line in my processing routine, I get the following:

$ ncinfo -v lon nofalse.nc <type 'netCDF4._netCDF4.Variable'> float64 lon(lon) _FillValue: nan units: degrees_east long_name: Longitude epic_code: 502 unlimited dimensions: current shape = (1,) filling on

I agree that changing from False to None does make better semantic sense.