PR Cleanup

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

PR Cleanup

Charles R Harris
Hi All,

As usual, the top of the PR stack is getting all the attention. As a start on cleaning up the old PRs, I'd like to suggest that all the maintainers look at their (long) pending PRs and decide which they want to keep, close those they don't want to pursue, and rebase the others. Might also help if they would post here the PRs that they think we should finish up.

Chuck

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

Re: PR Cleanup

Stefan van der Walt
On Tue, 25 Sep 2018 09:51:13 -0600, Charles R Harris wrote:
> As usual, the top of the PR stack is getting all the attention. As a start
> on cleaning up the old PRs, I'd like to suggest that all the maintainers
> look at their (long) pending PRs and decide which they want to keep, close
> those they don't want to pursue, and rebase the others. Might also help if
> they would post here the PRs that they think we should finish up.

PR 4808 [0] asks whether `np.pad` should have a default mode.  The
proposal is to make the defaul `constant`, with a value of 0.  This is
common in many signal processing applications.

Nathaniel requested that we ask the list before committing.

Best regards,
Stéfan


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

Re: PR Cleanup

Stephan Hoyer-2
On Tue, Sep 25, 2018 at 9:58 AM Stefan van der Walt <[hidden email]> wrote:
On Tue, 25 Sep 2018 09:51:13 -0600, Charles R Harris wrote:
> As usual, the top of the PR stack is getting all the attention. As a start
> on cleaning up the old PRs, I'd like to suggest that all the maintainers
> look at their (long) pending PRs and decide which they want to keep, close
> those they don't want to pursue, and rebase the others. Might also help if
> they would post here the PRs that they think we should finish up.

PR 4808 [0] asks whether `np.pad` should have a default mode.  The
proposal is to make the defaul `constant`, with a value of 0.  This is
common in many signal processing applications.

Nathaniel requested that we ask the list before committing.

+1 for constant padding by zero as the default mode. This is the only default behavior that I would guess.

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

Re: PR Cleanup

Robert Kern-2
In reply to this post by Stefan van der Walt
On Tue, Sep 25, 2018 at 9:59 AM Stefan van der Walt <[hidden email]> wrote:
PR 4808 [0] asks whether `np.pad` should have a default mode.  The
proposal is to make the defaul `constant`, with a value of 0.  This is
common in many signal processing applications.

+1 for that proposal. 

--
Robert Kern

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

Re: PR Cleanup

Charles R Harris


On Tue, Sep 25, 2018 at 11:08 AM Robert Kern <[hidden email]> wrote:
On Tue, Sep 25, 2018 at 9:59 AM Stefan van der Walt <[hidden email]> wrote:
PR 4808 [0] asks whether `np.pad` should have a default mode.  The
proposal is to make the defaul `constant`, with a value of 0.  This is
common in many signal processing applications.

+1 for that proposal. 


+1 also. If this is new behavior, needs a release note.

Chuck 

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

Re: PR Cleanup

Paul Hobson-2
In reply to this post by Stefan van der Walt
As a user, I found np.pad relatively complex to get up and running (with all of the string parameters and different formats of the parameters). Some defaults would be good to just prime the pump, so to speak, when users are tinkering in e.g., and interactive session.
-Paul

On Tue, Sep 25, 2018 at 9:59 AM Stefan van der Walt <[hidden email]> wrote:
On Tue, 25 Sep 2018 09:51:13 -0600, Charles R Harris wrote:
> As usual, the top of the PR stack is getting all the attention. As a start
> on cleaning up the old PRs, I'd like to suggest that all the maintainers
> look at their (long) pending PRs and decide which they want to keep, close
> those they don't want to pursue, and rebase the others. Might also help if
> they would post here the PRs that they think we should finish up.

PR 4808 [0] asks whether `np.pad` should have a default mode.  The
proposal is to make the defaul `constant`, with a value of 0.  This is
common in many signal processing applications.

Nathaniel requested that we ask the list before committing.

Best regards,
Stéfan


[0] https://github.com/numpy/numpy/pull/4808
_______________________________________________
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: PR Cleanup

Marten van Kerkwijk
In reply to this post by Charles R Harris
Hi Chuck,

Over at astropy we have a bot that sends a warning to PRs that have not received any commits for 5 months [1] and closes them if nothing happens in the next month, asking to open an issue instead. Despite my apprehensions, I found this worked quite well, drawing back attention to forgotten PRs (and forcing one to consider if they best remain forgotten).

Would there be interest in such a scheme?

All the best,

Marten

p.s. We don't do anything with issues.

[1] Text of the astropy-bot warning:

Hi humans wave - this pull request hasn't had any new commits for approximately 5 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. You may also consider sending a reminder e-mail about it to the astropy-dev mailing list.

If you believe I commented on this pull request incorrectly, please report this here.



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

Re: PR Cleanup

Joseph Fox-Rabinovitz
In reply to this post by Charles R Harris
I think that PRs 7804 and 10855 can be merged (or closed; closure either way).

7804: ENH: Added atleast_nd
    This has been ready to go for a while. It was deemed superfluous at one point, but then experienced a revival, which ended up stagnating.
10855: ENH: Adding a count parameter to np.unpackbits
    Has been ready for a while now as well. There is a possible modification to the interface that may need to be made (not allowing negative indices), but it's passing everything and good to go as-is, in my opinion.

Regards,

- Joe


On Tue, Sep 25, 2018 at 11:52 AM Charles R Harris <[hidden email]> wrote:
Hi All,

As usual, the top of the PR stack is getting all the attention. As a start on cleaning up the old PRs, I'd like to suggest that all the maintainers look at their (long) pending PRs and decide which they want to keep, close those they don't want to pursue, and rebase the others. Might also help if they would post here the PRs that they think we should finish up.

Chuck
_______________________________________________
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: PR Cleanup

ralfgommers
In reply to this post by Marten van Kerkwijk


On Tue, Sep 25, 2018 at 5:35 PM Marten van Kerkwijk <[hidden email]> wrote:
Hi Chuck,

Over at astropy we have a bot that sends a warning to PRs that have not received any commits for 5 months [1] and closes them if nothing happens in the next month, asking to open an issue instead. Despite my apprehensions, I found this worked quite well, drawing back attention to forgotten PRs (and forcing one to consider if they best remain forgotten).

Would there be interest in such a scheme?

My $2c: I've had this "bot experience" happen to me once (for a pip contribution), and that left a *really* bad taste. Reading the message below, that would do the same. E.g. no one bothered to review my PR in 5 months, and now you're asking me to rebase? If that's asked for, them much better if a human does it.

Other thought: there were a number of people uncomfortable with the pep8 bot we used to have. This one would be more intrusive.

Cheers,
Ralf

 

All the best,

Marten

p.s. We don't do anything with issues.

[1] Text of the astropy-bot warning:

Hi humans wave - this pull request hasn't had any new commits for approximately 5 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. You may also consider sending a reminder e-mail about it to the astropy-dev mailing list.

If you believe I commented on this pull request incorrectly, please report this here.


_______________________________________________
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: PR Cleanup

Allan Haldane
In reply to this post by Charles R Harris
On 09/25/2018 11:51 AM, Charles R Harris wrote:
> Hi All,
>
> As usual, the top of the PR stack is getting all the attention. As a
> start on cleaning up the old PRs, I'd like to suggest that all the
> maintainers look at their (long) pending PRs and decide which they want
> to keep, close those they don't want to pursue, and rebase the others.
> Might also help if they would post here the PRs that they think we
> should finish up.

PR 6377 [0] fixes up alignment bugs which cause complex64/128 to be
incorrectly aligned, which in turn causes further bugs. Eg,
"np.dtype('c8').alignment" is 8 but should be 4 on most systems.

The PR is in a finished state, modulo reviews. If anyone has access to
sparc/aarch systems it would be great to test it, though.

Cheers,
Allan


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

Re: PR Cleanup

Juan Nunez-Iglesias
In reply to this post by ralfgommers

On 26 Sep 2018, at 5:12 am, Ralf Gommers <[hidden email]> wrote:
My $2c: I've had this "bot experience" happen to me once (for a pip contribution), and that left a *really* bad taste.

I’m going to second Ralf’s comments here. These bots are a great idea if your number one goal is to reduce the number of open PRs at the cost of everything else — including happy contributors.

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

Re: PR Cleanup

Matthew Harrigan
In reply to this post by Allan Haldane
PR 8528 adds logical gufuncs such as " all equal".  The functionality has been mentioned quite a few times on this list. Many implementations are in the PR and they are decent IMHO.  The hard part is the API and current ufunc code.  Initially I thought they would be top level functions, ie np.all_equal, but there appears to be general consensus that they should be ufunc methods, ie np.equal.all.  Its not clear to me at least how that should be done.  Adding all, any, and first methods to all ufuncs seems bad.  Some sort of monkeypatch hack also seems bad.  Rewriting the ufunc code to make the methods specific to each ufunc, likely breaking the abi/api, is a big effort.  Suggestions welcome.  Thank you.

On Tue, Sep 25, 2018 at 6:56 PM Allan Haldane <[hidden email]> wrote:
On 09/25/2018 11:51 AM, Charles R Harris wrote:
> Hi All,
>
> As usual, the top of the PR stack is getting all the attention. As a
> start on cleaning up the old PRs, I'd like to suggest that all the
> maintainers look at their (long) pending PRs and decide which they want
> to keep, close those they don't want to pursue, and rebase the others.
> Might also help if they would post here the PRs that they think we
> should finish up.

PR 6377 [0] fixes up alignment bugs which cause complex64/128 to be
incorrectly aligned, which in turn causes further bugs. Eg,
"np.dtype('c8').alignment" is 8 but should be 4 on most systems.

The PR is in a finished state, modulo reviews. If anyone has access to
sparc/aarch systems it would be great to test it, though.

Cheers,
Allan


[0] https://github.com/numpy/numpy/pull/6377
_______________________________________________
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: PR Cleanup

Matthew Harrigan
In reply to this post by Juan Nunez-Iglesias
Would it be possible to have an opt-in for that bot?

On Tue, Sep 25, 2018 at 8:01 PM Juan Nunez-Iglesias <[hidden email]> wrote:

On 26 Sep 2018, at 5:12 am, Ralf Gommers <[hidden email]> wrote:
My $2c: I've had this "bot experience" happen to me once (for a pip contribution), and that left a *really* bad taste.

I’m going to second Ralf’s comments here. These bots are a great idea if your number one goal is to reduce the number of open PRs at the cost of everything else — including happy contributors.
_______________________________________________
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: PR Cleanup

Charles R Harris
In reply to this post by Matthew Harrigan


On Tue, Sep 25, 2018 at 6:09 PM Matthew Harrigan <[hidden email]> wrote:
PR 8528 adds logical gufuncs such as " all equal".  The functionality has been mentioned quite a few times on this list. Many implementations are in the PR and they are decent IMHO.  The hard part is the API and current ufunc code.  Initially I thought they would be top level functions, ie np.all_equal, but there appears to be general consensus that they should be ufunc methods, ie np.equal.all.  Its not clear to me at least how that should be done.  Adding all, any, and first methods to all ufuncs seems bad.  Some sort of monkeypatch hack also seems bad.  Rewriting the ufunc code to make the methods specific to each ufunc, likely breaking the abi/api, is a big effort.  Suggestions welcome.  Thank you.

Hmm, I'm not a big fan of ufunc methods, I think there are too many :) I suppose the argument in favor is decreasing the number of function names, which also has its proponents. What is the gain here in making them stand alone functions, more speed, possible SIMD speedups, etc.?

<snip>

Chuck  

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

Re: PR Cleanup

Matthew Harrigan
Speed, and to a lesser extent memory.  The biggest advantage is it allows short circuiting with ridiculous speedups for certain cases.  It also removes intermediate arrays/bandwidth.  There is some SIMD speedup but thats a relatively small benefit.

On Tue, Sep 25, 2018 at 8:18 PM Charles R Harris <[hidden email]> wrote:


On Tue, Sep 25, 2018 at 6:09 PM Matthew Harrigan <[hidden email]> wrote:
PR 8528 adds logical gufuncs such as " all equal".  The functionality has been mentioned quite a few times on this list. Many implementations are in the PR and they are decent IMHO.  The hard part is the API and current ufunc code.  Initially I thought they would be top level functions, ie np.all_equal, but there appears to be general consensus that they should be ufunc methods, ie np.equal.all.  Its not clear to me at least how that should be done.  Adding all, any, and first methods to all ufuncs seems bad.  Some sort of monkeypatch hack also seems bad.  Rewriting the ufunc code to make the methods specific to each ufunc, likely breaking the abi/api, is a big effort.  Suggestions welcome.  Thank you.

Hmm, I'm not a big fan of ufunc methods, I think there are too many :) I suppose the argument in favor is decreasing the number of function names, which also has its proponents. What is the gain here in making them stand alone functions, more speed, possible SIMD speedups, etc.?

<snip>

Chuck  
_______________________________________________
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: PR Cleanup

Marten van Kerkwijk
Hi Matthew,

With the decision not to support broadcasting in ufunc signatures, I think a ufunc route for generally useful all_equal has become hard at the present time. So, as it it seems the only useful route is indeed a separate function... (perhaps in the end we can use stackable ufuncs, which operate in chunks - I have a prototype but it doesn't do reductions yet, so not directly relevant).

All the best,

Marten

On Tue, Sep 25, 2018 at 8:25 PM Matthew Harrigan <[hidden email]> wrote:
Speed, and to a lesser extent memory.  The biggest advantage is it allows short circuiting with ridiculous speedups for certain cases.  It also removes intermediate arrays/bandwidth.  There is some SIMD speedup but thats a relatively small benefit.

On Tue, Sep 25, 2018 at 8:18 PM Charles R Harris <[hidden email]> wrote:


On Tue, Sep 25, 2018 at 6:09 PM Matthew Harrigan <[hidden email]> wrote:
PR 8528 adds logical gufuncs such as " all equal".  The functionality has been mentioned quite a few times on this list. Many implementations are in the PR and they are decent IMHO.  The hard part is the API and current ufunc code.  Initially I thought they would be top level functions, ie np.all_equal, but there appears to be general consensus that they should be ufunc methods, ie np.equal.all.  Its not clear to me at least how that should be done.  Adding all, any, and first methods to all ufuncs seems bad.  Some sort of monkeypatch hack also seems bad.  Rewriting the ufunc code to make the methods specific to each ufunc, likely breaking the abi/api, is a big effort.  Suggestions welcome.  Thank you.

Hmm, I'm not a big fan of ufunc methods, I think there are too many :) I suppose the argument in favor is decreasing the number of function names, which also has its proponents. What is the gain here in making them stand alone functions, more speed, possible SIMD speedups, etc.?

<snip>

Chuck  
_______________________________________________
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: PR Cleanup

Daπid
In reply to this post by ralfgommers


On Tue, 25 Sep 2018 at 21:14, Ralf Gommers <[hidden email]> wrote:


On Tue, Sep 25, 2018 at 5:35 PM Marten van Kerkwijk <[hidden email]> wrote:
Hi Chuck,

Over at astropy we have a bot that sends a warning to PRs that have not received any commits for 5 months [1] and closes them if nothing happens in the next month, asking to open an issue instead. Despite my apprehensions, I found this worked quite well, drawing back attention to forgotten PRs (and forcing one to consider if they best remain forgotten).

Would there be interest in such a scheme?

My $2c: I've had this "bot experience" happen to me once (for a pip contribution), and that left a *really* bad taste. Reading the message below, that would do the same. E.g. no one bothered to review my PR in 5 months, and now you're asking me to rebase? If that's asked for, them much better if a human does it.

Other thought: there were a number of people uncomfortable with the pep8 bot we used to have. This one would be more intrusive.

Cheers,
Ralf


I think Tensorflow does it better. Their butler-bot nags the core developer assigned to the PR or issue if the report isn't marked as awaiting information or suggestions pending. I think this is much nicer for external contributors. On the other hand, TF has paid staff to take care of this, here it would be up to you guys if you feel up to being nagged by robots.

Exact text:

Nagging Assignee @rohan100jain: It has been 14 days with no activity and this issue has an assignee. Please update the label and/or status accordingly.
 

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

Re: PR Cleanup

Matthew Harrigan
In reply to this post by Marten van Kerkwijk
Marten,
I think a ufunc route is pretty straightforward assuming there is a higher level function.  It would be something like this:

def wrapper_for_all_equal(x, y):
    return np.private_ufunc_all_equal(*np.broadcast_arrays(x, y))

The largest outstanding issue I see are what the api should be.  There was concern about adding too many functions to the already crowded numpy namespace.

On Tue, Sep 25, 2018 at 9:25 PM Marten van Kerkwijk <[hidden email]> wrote:
Hi Matthew,

With the decision not to support broadcasting in ufunc signatures, I think a ufunc route for generally useful all_equal has become hard at the present time. So, as it it seems the only useful route is indeed a separate function... (perhaps in the end we can use stackable ufuncs, which operate in chunks - I have a prototype but it doesn't do reductions yet, so not directly relevant).

All the best,

Marten

On Tue, Sep 25, 2018 at 8:25 PM Matthew Harrigan <[hidden email]> wrote:
Speed, and to a lesser extent memory.  The biggest advantage is it allows short circuiting with ridiculous speedups for certain cases.  It also removes intermediate arrays/bandwidth.  There is some SIMD speedup but thats a relatively small benefit.

On Tue, Sep 25, 2018 at 8:18 PM Charles R Harris <[hidden email]> wrote:


On Tue, Sep 25, 2018 at 6:09 PM Matthew Harrigan <[hidden email]> wrote:
PR 8528 adds logical gufuncs such as " all equal".  The functionality has been mentioned quite a few times on this list. Many implementations are in the PR and they are decent IMHO.  The hard part is the API and current ufunc code.  Initially I thought they would be top level functions, ie np.all_equal, but there appears to be general consensus that they should be ufunc methods, ie np.equal.all.  Its not clear to me at least how that should be done.  Adding all, any, and first methods to all ufuncs seems bad.  Some sort of monkeypatch hack also seems bad.  Rewriting the ufunc code to make the methods specific to each ufunc, likely breaking the abi/api, is a big effort.  Suggestions welcome.  Thank you.

Hmm, I'm not a big fan of ufunc methods, I think there are too many :) I suppose the argument in favor is decreasing the number of function names, which also has its proponents. What is the gain here in making them stand alone functions, more speed, possible SIMD speedups, etc.?

<snip>

Chuck  
_______________________________________________
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

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