Code review for adding axis argument to permutation and shuffle function

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Code review for adding axis argument to permutation and shuffle function

Kexuan Sun
Hi,

I would like to request a code review. The random.permutation and
random.shuffle functions now can only shuffle along the first axis of a
multi-dimensional array. I propose to add an axis argument for the
functions and allow them to shuffle along a given axis. Here is the link
to the PR (https://github.com/numpy/numpy/pull/13829).

Thanks!


_______________________________________________
NumPy-Discussion mailing list
[hidden email]
https://mail.python.org/mailman/listinfo/numpy-discussion
Reply | Threaded
Open this post in threaded view
|

Re: Code review for adding axis argument to permutation and shuffle function

mattip
Administrator
This proposal to add an axis argument to permutation and shuffle seems to
have garnered no reply. Are people OK with it (for the new random.Generator
only) ?



--
Sent from: http://numpy-discussion.10968.n7.nabble.com/
_______________________________________________
NumPy-Discussion mailing list
[hidden email]
https://mail.python.org/mailman/listinfo/numpy-discussion
Reply | Threaded
Open this post in threaded view
|

Re: Code review for adding axis argument to permutation and shuffle function

Juan Nunez-Iglesias-2
I don’t understand why the proposal would be controversial in any way. It’s very natural to have `axis=` keyword arguments in NumPy, and it’s the lack of them that is surprising. My only additional suggestion would be to allow tuples of axes, but that can come later.

Juan.

> On 13 Sep 2019, at 4:25 pm, mattip <[hidden email]> wrote:
>
> This proposal to add an axis argument to permutation and shuffle seems to
> have garnered no reply. Are people OK with it (for the new random.Generator
> only) ?
>
>
>
> --
> Sent from: http://numpy-discussion.10968.n7.nabble.com/
> _______________________________________________
> NumPy-Discussion mailing list
> [hidden email]
> https://mail.python.org/mailman/listinfo/numpy-discussion

_______________________________________________
NumPy-Discussion mailing list
[hidden email]
https://mail.python.org/mailman/listinfo/numpy-discussion
Reply | Threaded
Open this post in threaded view
|

Re: Code review for adding axis argument to permutation and shuffle function

Matthew Brett
Hi,

Thanks - yes - I agree, an axis argument seems like a very sensible idea.

Cheers,

Matthew

On Fri, Sep 13, 2019 at 7:48 AM Juan Nunez-Iglesias <[hidden email]> wrote:

>
> I don’t understand why the proposal would be controversial in any way. It’s very natural to have `axis=` keyword arguments in NumPy, and it’s the lack of them that is surprising. My only additional suggestion would be to allow tuples of axes, but that can come later.
>
> Juan.
>
> > On 13 Sep 2019, at 4:25 pm, mattip <[hidden email]> wrote:
> >
> > This proposal to add an axis argument to permutation and shuffle seems to
> > have garnered no reply. Are people OK with it (for the new random.Generator
> > only) ?
> >
> >
> >
> > --
> > Sent from: http://numpy-discussion.10968.n7.nabble.com/
> > _______________________________________________
> > NumPy-Discussion mailing list
> > [hidden email]
> > https://mail.python.org/mailman/listinfo/numpy-discussion
>
> _______________________________________________
> NumPy-Discussion mailing list
> [hidden email]
> https://mail.python.org/mailman/listinfo/numpy-discussion
_______________________________________________
NumPy-Discussion mailing list
[hidden email]
https://mail.python.org/mailman/listinfo/numpy-discussion
Reply | Threaded
Open this post in threaded view
|

Re: Code review for adding axis argument to permutation and shuffle function

Warren Weckesser-2
In reply to this post by Kexuan Sun
On 7/4/19, Kexuan Sun <[hidden email]> wrote:
> Hi,
>
> I would like to request a code review. The random.permutation and
> random.shuffle functions now can only shuffle along the first axis of a
> multi-dimensional array. I propose to add an axis argument for the
> functions and allow them to shuffle along a given axis. Here is the link
> to the PR (https://github.com/numpy/numpy/pull/13829).


Given the current semantics of 'shuffle', the proposed change makes
sense.  However, I would like to call attention to

    https://github.com/numpy/numpy/issues/5173

and to the mailing list thread from 2014 that I started here:

    https://mail.python.org/pipermail/numpy-discussion/2014-October/071340.html

The topic of those discussions was that the current behavior of
'shuffle' is often *not* what users want or expect.  What is often
desired is to shuffle each row (or column, or whatever dimension is
specified) *independently* of the others.  So if

     a = np.array([[0, 1, 2, 3, 4], [5, 6, 7, 8, 9], [10, 11, 12, 13, 14]]),

then randomly shuffling 'a' along axis=1 should shuffle each row
independently of the others, to create something like

     a = np.array([[2, 4, 0, 3, 1], [8, 6, 9, 7, 5], [11, 12, 10, 14, 13]])

An API for this was discussed (and of course that ran into the second
of the two hard problems in computer science, naming things).

Take a look at those discussions, and check that

    https://github.com/numpy/numpy/pull/13829

fits in with the possible changes mentioned in those discussions.  If
we don't use the name 'shuffle' for the new random permutation
function(s), then the change in PR 13829 is a good one.  However, if
we want to try to reuse the name 'shuffle' to also allow independent
shuffling along an axis, then we have to be careful with how we
interpret the 'axis' argument.


Warren


>
> Thanks!
>
>
> _______________________________________________
> NumPy-Discussion mailing list
> [hidden email]
> https://mail.python.org/mailman/listinfo/numpy-discussion
>
_______________________________________________
NumPy-Discussion mailing list
[hidden email]
https://mail.python.org/mailman/listinfo/numpy-discussion