OK, build passed so I'm merging. Thanks!

Crickey. Fixed merge hopefully it works (I hate merge conflicts)

Looks good. Unfortunately there was a merge conflict with the quantile PR so you need to merge master again.

OK added a performance improvements section to the docs

Actually, if you want to write that now I think we can squeeze this in. We are still a day or two away from the release.

On Mon, Jan 23, 2017 at 9:52 AM mangecoeur notifications@github.com wrote:

Note - waiting for 0.9.0 to be released before updating whats new, don't want to end up with conflicts in docs

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/1162#issuecomment-274564256, or mute the thread https://github.com/notifications/unsubscribe-auth/ABKS1iQJ4F8sn_fekpwRH-yDpJX6X0h0ks5rVOjhgaJpZM4LLMXX .

Note - waiting for 0.9.0 to be released before updating whats new, don't want to end up with conflicts in docs

Ok will wait for 0.9.0 to be released

This looks great to me. I'll merge this shortly after releasing 0.9.0 -- could you please add a brief release note in a new section for 0.9.1? (It won't be a long wait.)

Completed changes based on recommendations and cleaned up old code and comments.

As for benchmarks, I don't have anything rigourous but I do have the following example dataset weather data from the CFSR dataset, 7 variables at hourly resolution, collected in one netCDF3 file per variable per month. In the particular case the difference is striking!

python %%time data = dataset.isel_points(time=np.arange(0,1000), lat=np.ones(1000, dtype=int), lon=np.ones(1000, dtype=int)) data.load()

Results:

``` xarray 0.8.2 CPU times: user 1min 21s, sys: 41.5 s, total: 2min 2s Wall time: 47.8 s

master CPU times: user 385 ms, sys: 238 ms, total: 623 ms Wall time: 288 ms ```

@shoyer Tidied up based on recommendations, now everything done in a single loop (still need to make distinction between variables and coordinates for output but still a lot neater)

I believe Dataset.transpose already skips variables that don't need to be transposed. On Sat, Dec 24, 2016 at 5:40 AM mangecoeur notifications@github.com wrote:

@mangecoeur commented on this pull request.

In xarray/core/dataset.py https://github.com/pydata/xarray/pull/1162:

• coord_dim = var.dims[0] # should just be one?
• selection = take(var, indexers_dict[coord_dim]) +
• sel_coords[c] = (dim, selection)
• else:
• sel_coords[c] = self.coords[c] +
• variables = OrderedDict() +
• for name in data_vars:
• var = self.variables[name]

• Transpose the var to ensure that the indexed dims come first

• These dims will be collapsed in the output.

• To avoid edge cases in numpy want to transpose to ensure the indexed dimensions are first

• However transpose is not lazy, so want to avoid using it for dask case (??)

• var = var.transpose(*(list(d for d in indexer_dims if d in var.dims) +

I did that originally but then I thought that if there are variables that are not indexed at all we can skip applying transpose to them this way (and triggering a data load on them too). Does that make sense?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/1162, or mute the thread https://github.com/notifications/unsubscribe-auth/ABKS1j6axEaUtX-1kH3n_fmLy4o5ErRiks5rLRLHgaJpZM4LLMXX .

OK I adjusted for the new behaviour and all tests pass locally, hopefully travis agrees...

Edit: Looks like it's green

The development version of xarray includes a change that makes indexes optional. So if you use the Dataset/DataArray constructor it no longer adds new coordinates for each dimension by default.

@shoyer I'm down to 1 test failing locally in sel_points but not sure what the desired behaviour is. I get:

<xarray.Dataset> Dimensions: (points: 3) Coordinates: * points (points) int64 0 1 2 Data variables: foo (points) int64 0 4 8 instead of

AssertionError: <xarray.Dataset> Dimensions: (points: 3) Coordinates: o points (points) - Data variables: foo (points) int64 0 4 8

But here I'm not sure if my code is wrong or the test. It seems that the test requires sel_points NOT to generate a new coordinate values for points - however I'm pretty sure isel_points does require this (it passes in any case). Don't really see a way in my code to generate subsets without having a matching coordinate array (I don't know how to use the Dataset constructors without one for instance).

I've updated the test according to how I think it should be working, but please correct me if i misunderstood.

So it seems to work fine in the Dask case, but I don't have a deep understanding of how DataArrays are constructed from arrays and dims so it fails in the non-dask case. Also not sure how you feel about making a special case for the dask backend here (since up till now it was all backend agnostic).