Skip to content

Raise NotImplementedError when operating with numpy arrays #294

Merged
mrocklin merged 4 commits intodask:masterfrom
cowlicks:gh-290
Jun 12, 2015
Merged

Raise NotImplementedError when operating with numpy arrays #294
mrocklin merged 4 commits intodask:masterfrom
cowlicks:gh-290

Conversation

@cowlicks
Copy link
Member

Closes #290

This also fixes a test which had the numpy array and dask array flipped.

@cowlicks
Copy link
Member Author

I should add an assert_raises(NotImplementedError, ...) test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm -1 on changes like this. I personally find the meaning of the list comprehensions to be more evident. However I recognize that this is subjective; I wouldn't object to new code being written this way but prefer it if we don't go out of our way to rewrite existing code.

I suggest adding an extra line ahead of the two list comprehensions with the following check:

if any(isinstance(arg, np.ndarray) for arg in args):
    raise NotImplementedError("Dask.array operations only work on dask arrays, not numpy arrays.")

This isolates the type checking code from splitting code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rewrote this as one for loop to avoid looping over args three times. But this is really just a micro-optimization so I'll remove it.

@mrocklin
Copy link
Member

This is a good start but doesn't fully close #290. Something like this would need to be done in all dask.array functions into which a user might put a numpy.array. This is a great start though that probably solves 60% of the problem.

@cowlicks
Copy link
Member Author

@mrocklin would you like to merge this leaving #290 open? Or wait for other cases to be added to this PR and then close #290 with it?

@cowlicks
Copy link
Member Author

Or we could add multiple dispatch to NumPy. numpy/numpy#5844 (comment)

@shoyer
Copy link
Member

shoyer commented Jun 11, 2015

@cowlicks __numpy_ufunc__ will get us a large portion of the way there, but it can't handle all dispatching cases (e.g., it can't properly dispatch on lists of arrays, as needed for concatenate). I agree that multiple dispatch of some sort is the right path forward here in the long term (as do all the other commenters in that linked thread, I believe, especially @njsmith), but in the short term we're not there yet.

mrocklin added a commit that referenced this pull request Jun 12, 2015
Raise NotImplementedError when operating with numpy arrays
@mrocklin mrocklin merged commit 9885511 into dask:master Jun 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve user experience of interacting with numpy arrays

3 participants