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

somes fixes for the roll function #273

Merged
merged 4 commits into from Apr 1, 2013

Conversation

Projects
None yet
3 participants
Contributor

dlax commented May 8, 2012

  • added tests for roll (based on examples in the docstring)
  • raise a ValueError when axis is out of range
  • handle empty arrays (currenlty, np.roll(np.array([]), 1) results in a ZeroDivisionError)

@charris charris and 1 other commented on an outdated diff May 20, 2012

numpy/core/numeric.py
reshape = False
- shift %= n
- indexes = concatenate((arange(n-shift,n),arange(n-shift)))
- res = a.take(indexes, axis)
- if reshape:
- return res.reshape(a.shape)
+ if n == 0:
+ return a
else:
@charris

charris May 20, 2012

Owner

Don't really need the else since the other branch returns.

@dlax

dlax May 21, 2012

Contributor

Charles Harris wrote:

Don't really need the else since the other branch returns.

I tend to believe that the else makes things more readable in general
but I can change this if you prefer. Is it to minimize the overall diff?

@charris

charris May 21, 2012

Owner

Nah, just a comment. I think it is fine as is.

@charris

charris May 21, 2012

Owner

Although there is some advantage to keeping the indentation down. I tend to see this sort of thing as a check for a special case and I think those look better as a series of 'if's.

@charris charris commented on an outdated diff May 20, 2012

numpy/core/numeric.py
else:
- return res
+ shift %= n
+ indexes = concatenate((arange(n-shift,n),arange(n-shift)))
@charris

charris May 20, 2012

Owner

Spaces around operators and after ','.

@charris charris commented on an outdated diff May 20, 2012

numpy/core/numeric.py
else:
- return res
+ shift %= n
+ indexes = concatenate((arange(n-shift,n),arange(n-shift)))
+ res = a.take(indexes, axis)
+ if reshape:
+ return res.reshape(a.shape)
@charris

charris May 20, 2012

Owner

Could do

res = res.reshape(a.shape)

And get rid of the else, but it doesn't really matter much here.

Owner

charris commented May 20, 2012

Looks good to me apart from minor style things.

Owner

charris commented May 21, 2012

I would also like to see a C inplace version of this sometime. A rotation is the product of two reflections so it isn't that hard to do.

Contributor

dlax commented Oct 13, 2012

Charles Harris wrote:

Looks good to me apart from minor style things.

8d83ae9 fixes the style.

I would also like to see a C inplace version of this sometime. A rotation is the product of two reflections so it isn't that hard to do.

Unfortunately, it is unlikely that I'll do this soon as I don't know enough of numpy's internals...

Member

seberg commented Oct 13, 2012

Maybe stupid question, but is there a reason why this concatenates indices instead of concatenating slices of the original array directly? Admittingly you need to build a slice tupls like:

slobj1 = [None] * a.ndim
slobj2 = [None] * a.ndim
slobj1[axis] = slice(n-shift, n)
slobj2[axis] = slice(0, n-shift)
res = np.concatenate(a[slobj1], a[slobj2], axis=axis)

but using take seems not awesome to me. Hmmm, Need to use asarray and then wrap by hand though in case users overwrite getitem I think, like np.delete, etc. do.

Owner

charris commented Apr 1, 2013

@seberg Looks like this just fixes a bug in the current function without making any improvements, so I am going to put it in. The improvements would be a separate PR>

@charris charris added a commit that referenced this pull request Apr 1, 2013

@charris charris Merge pull request #273 from dlax/fix/roll
somes fixes for the roll function
a6385b3

@charris charris merged commit a6385b3 into numpy:master Apr 1, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment