Only fuse simple indexing with getarray backends by jcrist · Pull Request #1529 · dask/dask

jcrist · 2016-09-07T23:49:47Z

Certain numpy-array-like-things may not support all of the indexing methods that numpy uses. When fusing getitem/getarray calls together, we shouldn't pass "fancy indexing" requests to these backends. The easiest way to do this is to not propogate fusing of these types back inside calls to getarray.

This PR includes the following changes:

Don't fuse indexing using lists or arrays inside calls to getarray.
Make from_array use getitem when used with numpy types that return
instances of ndarray when indexed. This is a small optimization to
make use of np.memmap and np.ndarray still as fast as before.

Fixes #1528.

In [1]: import zarr

In [2]: import numpy as np

In [3]: import dask.array as da

In [4]: x = np.random.randint(0, 4, size=(10000, 100))

In [5]: f = np.random.randint(0, 2, size=10000).astype(bool)

In [6]: zx = zarr.array(x)

In [7]: dzx = da.from_array(zx, chunks=zx.chunks)

In [8]: res = da.compress(f, dzx, axis=0)

In [9]: res.compute()
Out[9]:
array([[0, 3, 0, ..., 1, 3, 0],
       [0, 1, 0, ..., 3, 2, 3],
       [0, 2, 2, ..., 2, 1, 3],
       ...,
       [2, 3, 3, ..., 0, 2, 2],
       [3, 0, 0, ..., 1, 0, 0],
       [3, 0, 3, ..., 3, 0, 1]])

Certain numpy-array-like-things may not support all of the indexing methods that numpy uses. When fusing getitem/getarray calls together, we shouldn't pass "fancy indexing" requests to these backends. The easiest way to do this is to not propogate fusing of these types back inside calls to `getarray`. This PR includes the following changes: - Don't fuse indexing using lists or arrays inside calls to `getarray`. - Make `from_array` use `getitem` when used with numpy types that return instances of `ndarray` when indexed. This is a small optimization to make use of `np.memmap` and `np.ndarray` still as fast as before.

jcrist · 2016-09-07T23:57:52Z

Oop, turns out slicing np.memmap objects returns a np.memmap, so this PR isn't quite good due to my getarray -> getitem swap for those. Before I revert that, it might be nice to consider a third getter function that allows these rewrites but still converts to np.ndarray internally. So we'd have:

getitem: no conversion, allows rewrites
getarray: conversion, no rewrites
safe_getarray: conversion, allows rewrites

Depending on backend, from_array could then use getarray or safe_getarray appropriately.
Alternatively, a parameter safe could be added to getarray, and the rewrite could check that to see if it's valid. Either works.

shoyer · 2016-09-08T00:02:25Z

As is, this would be a significant performance regression for xarray. We need a way to be able to call da.from_array with these optimizations on non-numpy arrays.

jcrist · 2016-09-08T00:04:41Z

Would exposing safe=True (with a better keyword name) as a keyword on from_array be good, or would you rather have an extensible dispatch (more work for us) to determine which get* to use in from_array for each type?

shoyer · 2016-09-08T00:11:35Z

I think a keyword argument would suffice -- an extensible dispatch system
is overkill at this point. We can inspect the types of input arrays on our
end and call from_array properly instead xarray's chunk method.

On Wed, Sep 7, 2016 at 5:04 PM, Jim Crist [email protected] wrote:

Would exposing safe=True (with a better keyword name) as a keyword on
from_array be good, or would you rather have an extensible dispatch (more
work for us) to determine which get* to use in from_array for each type?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1529 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABKS1rkTDmtjFIsj7z2RA4PRVMYCmzO3ks5qn1EagaJpZM4J3eN_
.

alimanfoo · 2016-09-08T09:57:33Z

Just out of interest, what set of fancy indexing features do you make use of in the case where the backend is numpy array?

FWIW I'd be happy to use a keyword argument if that's easier. Maybe "fancy=True" if the intention is to specify whether the backend supports fancy indexing?

shoyer · 2016-09-08T16:01:22Z

Just out of interest, what set of fancy indexing features do you make use of in the case where the backend is numpy array?

In the simplest case, just pulling out a single value from some axes of an array, e.g., x[0, 0, :] (not even fancy indexing) makes use of fusing. This sort of behavior works with almost any backend (including netCDF4, h5py, etc.), even if it doesn't support full fancy indexing (which is the case for both of those), and often it yields major performance benefits because you no longer need to fully chunk the array ahead of time.

alimanfoo · 2016-09-08T16:52:04Z

Zarr supports this, I.e., indexing with slices and/or integers. It doesn't
support anything else, eg, Boolean array or list/array of indices.

I asked because if dask needs only a limited subset of fancy indexing
features in the backend then I was thinking it might be doable to implement
in Zarr. But on reflection I think that would be a stretch, I'd rather keep
Zarr to just slices and ints, at least for now.

On Thursday, 8 September 2016, Stephan Hoyer [email protected]
wrote:

Just out of interest, what set of fancy indexing features do you make use
of in the case where the backend is numpy array?

In the simplest case, just pulling out a single value from some axes of an
array, e.g., x[0, 0, :](not even fancy indexing) makes use of fusing.
This sort of behavior works with almost any backend (including netCDF4,
h5py, etc.), even if it doesn't support full fancy indexing (which is the
case for both of those), and often it yields major performance benefits
because you no longer need to fully chunk the array ahead of time.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1529 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAq8Ql-URyyo0K9Wc4muefesxQi-pIdVks5qoDFVgaJpZM4J3eN_
.

Alistair Miles
Head of Epidemiological Informatics
Centre for Genomics and Global Health http://cggh.org
The Wellcome Trust Centre for Human Genetics
Roosevelt Drive
Oxford
OX3 7BN
United Kingdom
Email: [email protected]
Web: http://purl.org/net/aliman
Twitter: https://twitter.com/alimanfoo
Tel: +44 (0)1865 287721

shoyer · 2016-09-08T16:54:27Z

Zarr supports this, I.e., indexing with slices and/or integers. It doesn't support anything else, eg, Boolean array or list/array of indices.

In that case this isn't even a good fix for Zarr. We need to not fuse fancy indexing operations, but integers/slices should still be fused.

`da.from_array` now takes the keyword `fancy=True` to indicate whether the datastore supports "fancy indexing". The default is True, keeping with current behavior. If `False`, the rewrites will no longer fuse indexing involving fancy indexing inside of calls to `getarray_nofancy`.

jcrist · 2016-09-08T19:05:59Z

I added a keyword fancy to da.from_array, to indicate whether the backend supports fancy indexing. The default is True, keeping with current behavior. If False, no fancy indexing will be pushed inside of a call to getarray_nofancy during the optimization pass.

Example using zarr:

import zarr
import dask.array as da
import numpy as np

x = np.random.randint(0, 4, size=(10000, 100))
zx = zarr.array(x)
dzx = da.from_array(zx, chunks=zx.chunks, fancy=False)
dzx[[1, 2]].compute() # This works fine

dzx = da.from_array(zx, chunks=zx.chunks)  # don't pass in `Fancy=False`
dzx[[1, 2]].compute()  # This errors

jcrist · 2016-09-09T15:28:23Z

Planning to merge later today if no comment.

shoyer · 2016-09-09T15:43:45Z

+        pass in True to have dask.array create one for you.
+    fancy : bool, optional
+        If ``x`` doesn't support fancy indexing (e.g. indexing with lists or
+        arrays) then set to False. Default is True.


"Default is True." is inconsistent with the function signature (one reason why you never put default argument values in docstrings)

def from_array(x, chunks, name=None, lock=False, fancy=True)

The default for fancy is True.

shoyer · 2016-09-09T15:44:48Z

I can't say I follow the logic of the fusing operations, but given that this is backwards compatible it seems safe to me.

jcrist mentioned this pull request Sep 7, 2016

da.compress() error with zarr array #1528

Closed

shoyer reviewed Sep 9, 2016
View reviewed changes

jcrist merged commit 1aacb84 into dask:master Sep 9, 2016

jcrist deleted the dont-fuse-lists-in-getarray branch September 9, 2016 17:08

sinhrks added this to the 0.11.1 milestone Oct 9, 2016

laliberte mentioned this pull request Nov 11, 2016

Several modifications to fix slicing bug #1776

Closed

Conversation

jcrist commented Sep 7, 2016

Uh oh!

jcrist commented Sep 7, 2016

Uh oh!

shoyer commented Sep 8, 2016

Uh oh!

jcrist commented Sep 8, 2016

Uh oh!

shoyer commented Sep 8, 2016

Uh oh!

alimanfoo commented Sep 8, 2016

Uh oh!

shoyer commented Sep 8, 2016

Uh oh!

alimanfoo commented Sep 8, 2016

Uh oh!

shoyer commented Sep 8, 2016

Uh oh!

jcrist commented Sep 8, 2016

Uh oh!

jcrist commented Sep 9, 2016

Uh oh!

shoyer Sep 9, 2016

Choose a reason for hiding this comment

Uh oh!

jcrist Sep 9, 2016

Choose a reason for hiding this comment

Uh oh!

shoyer Sep 9, 2016

Choose a reason for hiding this comment

Uh oh!

shoyer commented Sep 9, 2016

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants