Generalized flip #7346

erensezener · 2016-02-26T16:14:10Z

This PR generalizes fliplr and flipud for arbitrary axes.

flipud and fliplr reverse the elements of an array along axis=0 and axis=1 respectively. The flip function reverses the elements of an array along any given axis. In case flip is called with axis=0 or axis=1, the function is equivalent to flipud and fliplr respectively.

A similar function is also available in MATLAB™.

This pull request is part of a BCCN Berlin student project by @denisalevi @ge00rg @erensezener

shoyer · 2016-02-26T16:17:35Z

I think this is a nice idea, but we have a policy that new API needs to be discussed on the mailing list. Could you please post your proposal to the numpy-discussion list: https://mail.scipy.org/mailman/listinfo/numpy-discussion

Thanks!

madphysicist · 2016-02-26T16:47:17Z

numpy/lib/tests/test_twodim_base.py

+                    [7, 6]]])
+
+        assert_equal(flip(a, 2), b)
+


I think you should add a test for a flip at higher dimensions. If you don't want to construct the input array and expected result manually, perhaps show something like

a = np.arange(2 * 3 * 4 * 5).reshape(2, 3, 4, 5) for i in range(a.ndim): assert_equal(flip(a, i), np.flipud(a.swapaxes(0, i)).swapaxes(i, 0))

This will also implicitly demonstrate the claim that the shape of the array is preserved, although you can add an explicit check. All your other test arrays are 2**3, so it would be hard to tell if the dimensions moved around.

Added your test example here: 468c4f2

madphysicist · 2016-02-26T16:58:37Z

numpy/lib/twodim_base.py

+    >>> np.all(flip(A,2)==A[:,:,::-1,...])
+    True
+    """
+    if axis==0:


Are these special cases really necessary? The function seems to be able to handle them already.

They are not necessary but flipud and fliplr should be slightly faster. Should we remove the cases?

In general, clarity is more important than unmeasured micro-optimizations.

On Fri, Feb 26, 2016 at 3:10 PM, C. Eren Sezener
notifications@github.com wrote:

In numpy/lib/twodim_base.py:

[[0, 1],

[2, 3]]])

flip(A, 1)

array([[[2, 3],

[0, 1]],

[[6, 7],

[4, 5]]])

A = np.random.randn(3,4,5)

np.all(flip(A,2)==A[:,:,::-1,...])

True

"""

if axis==0:

They are not necessary but flipud and fliplr should be slightly faster.
Should we remove the cases?

—
Reply to this email directly or view it on GitHub.

Nathaniel J. Smith -- https://vorpus.org

Removed the special cases 4443294

The special cases increase the speed for axis = 0 and axis = 1 by a factor of approximately 1.5.

Measuring the execution time of flip(a, 0) using timeit and for an array a = np.arange(100**4).reshape(100,100,100,100) I got (from 1000000 loops, best of 3):

0.865 us when using the special cases (returning flipud(a))
1.34 us when using the current implementation from abe1735 using @njsmith suggestion.

Does that still fall into micro-optimization?

Yes. 0.5 us is very, very fast.
On Fri, Feb 26, 2016 at 11:32 PM Denis Alevi notifications@github.com
wrote:

In numpy/lib/twodim_base.py
#7346 (comment):

[[0, 1],

[2, 3]]])

flip(A, 1)

array([[[2, 3],

[0, 1]],

[[6, 7],

[4, 5]]])

A = np.random.randn(3,4,5)

np.all(flip(A,2)==A[:,:,::-1,...])

True

"""

if axis==0:

The special cases increase the speed for axis = 0 and axis = 1 by a
factor of approximately 1.5.

Measuring the execution time of flip(a, 0) using timeit and for an array a
= np.arange(100**4).reshape(100,100,100,100) I got (from 1000000 loops,
best of 3):

0.865 us when using the special cases (returning flipud(a))
1.34 us when using the current implementation from abe1735
erensezener@abe1735
using @njsmith https://github.com/njsmith suggestion.

Does that still fall into micro-optimization?

—
Reply to this email directly or view it on GitHub
https://github.com/numpy/numpy/pull/7346/files#r54328158.

madphysicist · 2016-02-26T17:07:43Z

Add a note in the release notes. Aside from the possible bug and other comments, nice job, this looks like a really handy addition.

madphysicist · 2016-02-26T17:11:28Z

numpy/lib/twodim_base.py

@@ -32,6 +32,76 @@ def _min_int(low, high):
    return int64


+def flip(m, axis):


Since this is a function that works on arbitrary dimensions, not just 2-d, is this the right file to put it? Perhaps function_base.py and tests/test_function_base.py would be a better choice?

njsmith · 2016-02-26T17:33:55Z

Should flipud and fliplr be rewritten in terms of this?

mhvk · 2016-02-26T17:41:26Z

In principle, this could be made even more general by allowing an iterable for axis (i.e., flipping over multiple axes at the same time). Probably overkill, though.

erensezener · 2016-02-27T00:43:44Z

I think I addressed all the issues pointed out besides @madphysicist's last comment:

Since this is a function that works on arbitrary dimensions, not just 2-d, is this the right file to put it? Perhaps function_base.py and tests/test_function_base.py would be a better choice?

Should I also change the file of the function? What are other opinions?

mhvk · 2016-02-27T02:30:38Z

numpy/lib/twodim_base.py

+    """
+    if not hasattr(m, 'ndim'):
+        m = asarray(m)
+    if m.ndim <= axis:


This doesn't cover negative values of axis. I would remove this altogether and replace it by making the assignment part of a try/except, as follows:

try: indexer[axis] = slice(None, None, -1) except IndexError: raise ValueError(...)

Question: Should this be an IndexError rather than a ValueError?

No, ValueError is definitely the right error to raise for an invalid axis.
On Fri, Feb 26, 2016 at 9:30 PM Marten van Kerkwijk <
notifications@github.com> wrote:

In numpy/lib/twodim_base.py
#7346 (comment):

[2, 3]]])

flip(A, 1)

array([[[2, 3],

[0, 1]],

[[6, 7],

[4, 5]]])

A = np.random.randn(3,4,5)

np.all(flip(A,2)==A[:,:,::-1,...])

True

"""

if not hasattr(m, 'ndim'):

m = asarray(m)

if m.ndim <= axis:

This doesn't cover negative values of axis. I would remove this
altogether and replace it by making the assignment part of a try/except, as
follows:

try: indexer[axis] = slice(None, None, -1) except IndexError: raise ValueError(...)

Question: Should this be an IndexError rather than a ValueError?

—
Reply to this email directly or view it on GitHub
https://github.com/numpy/numpy/pull/7346/files#r54326237.

Also, I would recommend adding one more exception test that passes in axis < -m.ndim.

added negative axis support with try/except for index error here bc8ec6b

and an exception test here 3dd2a56

erensezener · 2016-02-28T12:30:34Z

As far as I can see, @denisalevi handled all the remaining issues. Ready for merge?

shoyer · 2016-02-28T18:01:20Z

numpy/lib/twodim_base.py

+    -------
+    out : array_like
+        A view of `m` with the entries of axis reversed.  Since a view is
+        returned, this operation is :math:`\\mathcal O(1)`.


I would simply say "constant time".

Done e10be0e

charris · 2016-03-02T23:43:47Z

Updated.

charris · 2016-03-02T23:44:41Z

Needs mention in doc/release/1.12.0-notes.rst.

charris · 2016-03-02T23:47:59Z

Also squash the 12 commits into one or two. You can use git rebase -i HEAD~24 for that followed by a force push git push --force origin <your branch>.

shoyer · 2016-03-03T00:43:55Z

numpy/lib/twodim_base.py

+        indexer[axis] = slice(None, None, -1)
+    except IndexError:
+        raise ValueError("axis=%i is invalid for the %i-dimensional input array"
+                         %(axis, m.ndim))


per pep8, please add a space after the % operator

erensezener · 2016-03-03T13:00:12Z

@charris under what title should the release note be? New features?

denisalevi · 2016-03-12T14:14:35Z

@charris We moved the files and squashed the commits. If you prefer a single PR for flip and rot90, the rot90 PR (#7247) includes the changes of this one here (the generalized_rot90 branch is rebased on the generalized_flip branch)

charris · 2016-03-12T15:29:15Z

doc/release/1.12.0-notes.rst

@@ -99,6 +99,10 @@ keyword argument. It can be set to False when no write operation
 to the returned array is expected to avoid accidental
 unpredictable writes.

+Generalized ``flip``
+~~~~~~~~~~~~~~~~~~~~
+``flipud`` and ``fliplr`` reverse the elements of an array along axis=0 and axis=1 respectively. The newly added ``flip`` function reverses the elements of an array along any given axis.


Lines less than 80 characters in length, need line breaks here.

charris · 2016-03-12T15:35:14Z

Getting close. Add an entry in doc/source/reference/routines.array-manipulation.rst also. Search for flipud to see where. The test failures are bogus, so don't worry about them.

denisalevi · 2016-03-12T18:10:37Z

@charris done and squashed.

charris · 2016-03-12T19:34:23Z

numpy/lib/tests/test_function_base.py

+    def test_3d_swap_axis0(self):
+        a = np.array([[[0, 1],
+                       [2, 3]],
+


Can leave out the blank lines.

charris · 2016-03-12T19:36:57Z

I've made a few comments in the tests for formatting. I'll put this in and you can do the fixes in the rot90 PR.

Generalized flip

charris · 2016-03-12T20:04:22Z

Thanks @erensezener and co.

erensezener · 2016-03-12T21:25:53Z

Thanks @charris. Did the changes here: 7a57783d84188d24ed18dc16a825b13a556087d3

erensezener mentioned this pull request Feb 26, 2016

ENH Generalized rot90 #7347

Merged

madphysicist reviewed Feb 26, 2016
View reviewed changes

charris added 01 - Enhancement component: numpy.lib labels Feb 26, 2016

madphysicist reviewed Feb 26, 2016
View reviewed changes

charris added the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Feb 26, 2016

madphysicist reviewed Feb 26, 2016
View reviewed changes

mhvk reviewed Feb 27, 2016
View reviewed changes

shoyer reviewed Feb 28, 2016
View reviewed changes

shoyer reviewed Mar 3, 2016
View reviewed changes

erensezener force-pushed the generalized_flip branch 2 times, most recently from c619a79 to 4333f98 Compare March 9, 2016 19:21

denisalevi force-pushed the generalized_flip branch from 4333f98 to 5e0de07 Compare March 11, 2016 14:10

erensezener force-pushed the generalized_flip branch from 87f5ea1 to 7614188 Compare March 11, 2016 22:04

denisalevi force-pushed the generalized_flip branch from 7614188 to 5530793 Compare March 12, 2016 13:56

charris reviewed Mar 12, 2016
View reviewed changes

ENH: Add generalized flip function and its tests

e7de401

denisalevi force-pushed the generalized_flip branch from 5530793 to e7de401 Compare March 12, 2016 18:08

charris removed the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Mar 12, 2016

charris reviewed Mar 12, 2016
View reviewed changes

numpy/lib/tests/test_function_base.py

def test_3d_swap_axis0(self):

a = np.array([[[0, 1],

[2, 3]],

Copy link

Member

charris Mar 12, 2016

Choose a reason for hiding this comment

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

Can leave out the blank lines.

charris added a commit that referenced this pull request Mar 12, 2016

Merge pull request #7346 from erensezener/generalized_flip

5b91628

Generalized flip

charris merged commit 5b91628 into numpy:master Mar 12, 2016

homu mentioned this pull request Mar 12, 2016

ENH: added axis param for np.count_nonzero #7177

Merged

		@@ -32,6 +32,76 @@ def _min_int(low, high):
		return int64


		def flip(m, axis):

Generalized flip #7346

Generalized flip #7346

Conversation

erensezener commented Feb 26, 2016

shoyer commented Feb 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madphysicist commented Feb 26, 2016

Choose a reason for hiding this comment

njsmith commented Feb 26, 2016

mhvk commented Feb 26, 2016

erensezener commented Feb 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erensezener commented Feb 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charris commented Mar 2, 2016

charris commented Mar 2, 2016

charris commented Mar 2, 2016

Choose a reason for hiding this comment

erensezener commented Mar 3, 2016

denisalevi commented Mar 12, 2016

Choose a reason for hiding this comment

charris commented Mar 12, 2016

denisalevi commented Mar 12, 2016

Choose a reason for hiding this comment

charris commented Mar 12, 2016

charris commented Mar 12, 2016

erensezener commented Mar 12, 2016