Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

ENH: Allow 0-d indexes in np.take #2725

Merged
merged 1 commit into from Jan 23, 2013

Conversation

Projects
None yet
3 participants
Member

seberg commented Nov 13, 2012

The TakeFrom already supported this. This removes the check which made it not possible and adds some tests for take.

There may well be a good reason for it, and maybe this is too central function to make it suddenly work where it used to not work. But it puzzled me, since take accepts N-d array but specifically not 0-d.

@njsmith njsmith and 1 other commented on an outdated diff Nov 13, 2012

numpy/core/tests/test_item_selection.py
@@ -0,0 +1,43 @@
+import numpy as np
+from numpy.testing import *
+import sys, warnings
+
+def test_take(level=1):
@njsmith

njsmith Nov 13, 2012

Owner

What is the level=1 argument?

@seberg

seberg Nov 13, 2012

Member

woops, I was wondering what level was and forgot to remove it.

Owner

njsmith commented Nov 13, 2012

Also, just to check that I understand, with this change we will have

>>> np.take([10, 20, 30], 1)
20

(and not an error as now, and also not anything like

>>> np.take([10, 20, 30], 1)
[20]

?)

Member

seberg commented Nov 13, 2012

yes exactly.

@njsmith njsmith commented on an outdated diff Nov 13, 2012

numpy/core/tests/test_item_selection.py
+ a = [[1, 2], [3, 4]]
+ a_str = [['1','2'],['3','4']]
+ modes = ['raise', 'wrap', 'clip']
+ indices = [-1, 4]
+ index_arrays = [np.empty(0, dtype=np.intp),
+ np.empty(tuple(), dtype=np.intp),
+ np.empty((1,1), dtype=np.intp)]
+ real_indices = {}
+ real_indices['raise'] = {-1:1, 4:IndexError}
+ real_indices['wrap'] = {-1:1, 4:0}
+ real_indices['clip'] = {-1:0, 4:1}
+ # Currently all types but object, use the same function
+ # generation. So it should not be necessary to test all.
+ types = np.int, np.object
+ for t in types:
+ ta = np.array(a if issubclass(t, np.number) else a_str, dtype=t)
@njsmith

njsmith Nov 13, 2012

Owner

Travis failure is legitimate: https://travis-ci.org/numpy/numpy/jobs/3172720

The ternary operator here isn't supported in Python 2.4.

Owner

charris commented Dec 3, 2012

Looks good to me. The documentation needs a note about the change, maybe in the indices parameter, with .. versionadded:: 1.8.0. You can grep on versionadded for examples. Also add something to doc/release/1.8.0-notes.rst. I don't see how adding this functionality will cause problems, so I'm inclined to put it in.

Member

seberg commented Dec 3, 2012

@charris thanks started on that, just got a bit of a question. I somewhat expected things take to always return an array and not a scalar. However it actually has the code in place to convert the 0-d array to a scalar on the python side (not that it could have had an effect before this). So I guess I will just leave that, or does anyone think that np.take should always return an array, which would be different from fancy-indices?

Owner

njsmith commented Dec 3, 2012

This kind of "API taste" question is probably better posed to the mailing
list, or by tracking down a few actual users...

On Mon, Dec 3, 2012 at 10:33 PM, seberg notifications@github.com wrote:

@charris https://github.com/charris thanks started on that, just got a
bit of a question. I somewhat expected things take to always return an
array and not a scalar. However it actually has the code in place to
convert the 0-d array to a scalar on the python side (not that it could
have had an effect before this). So I guess I will just leave that, or does
anyone think that np.take should always return an array, which would be
different from fancy-indices?


Reply to this email directly or view it on GitHubhttps://github.com/numpy/numpy/pull/2725#issuecomment-10975450.

ENH: Allow 0-d indexes in np.take
The TakeFrom already supported this. This removes the check which
made it not possible and adds some tests for take.

Also add documentation and information to the release notes.
Member

seberg commented Jan 6, 2013

OK, back then there was almost no response, so I guess it should return a scalar (as it does with this). There is one small unless here, and that is: unless we want to guarantee returning an array if the input is a 0-d array. This is something numpy never does (also for indexing):

In [1]: type(np.ones(10)[0])
Out[1]: numpy.float64

In [2]: type(np.ones(10)[np.array(0)])
Out[2]: numpy.float64

And quite honestly its not really worth the trouble to conserve arrays, 0-d arrays should be so rare and are so rarely conserved. This is only maybe something to maybe keep in mind if the scalar machinery gets reworked anyway.

So what I am saying I think behaving like indexing in this regard is the right thing. (Otherwise the sentence in the release notes is changed and the python 2.4 support removed in the rebase)

Owner

njsmith commented Jan 6, 2013

👍

Member

seberg commented Jan 22, 2013

btw. did that 👍 mean I should/can press the big green button? :)

Owner

njsmith commented Jan 22, 2013

well, I guess no-one else has objected, so, yes :-)

seberg added a commit that referenced this pull request Jan 23, 2013

Merge pull request #2725 from seberg/take_0d
ENH: Allow 0-d indexes in np.take

@seberg seberg merged commit dce1018 into numpy:master Jan 23, 2013

1 check passed

default The Travis build passed
Details

@seberg seberg deleted the seberg:take_0d branch Jan 23, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment