Skip to content

DEP: Deprecate the keepdims argument to accumulate #8198

New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 23, 2016
Merged

DEP: Deprecate the keepdims argument to accumulate #8198

merged 1 commit into from
Oct 23, 2016

Conversation

eric-wieser
Copy link
Member

As discussed in #8187

@eric-wieser eric-wieser mentioned this pull request Oct 22, 2016
Merged
@eric-wieser eric-wieser force-pushed the warning-on-accumulate-keepdim branch 2 times, most recently from c2901c6 to bcc4025 Compare October 22, 2016 02:23
@charris
Copy link
Member

charris commented Oct 22, 2016

OK, looks good. Two more things, add an entry to the doc/release/1.12.0-notes.rst mentioning the deprecation and then put a test in numpy/cores/tests/test_deprecations.py. It is also helpful if the numpy version and date are in a comment before the deprecation so that it can be searched for and checked when the time comes. Something like

/* 2016-10-21, 1.12 */

@charris
Copy link
Member

charris commented Oct 22, 2016

At this point you must feel like you are getting nickle and dimed to death ;) Thanks for your patience. Changing arguments/behavior is an ambitious thing...

charris
charris reviewed Oct 22, 2016
View reviewed changes
numpy/core/src/umath/ufunc_object.c
@@ -3891,18 +3894,29 @@ PyUFunc_GenericReduction(PyUFuncObject *ufunc, PyObject *args,
}
}
else if (operation == UFUNC_ACCUMULATE) {
if (!PyArg_ParseTupleAndKeywords(args, kwds, "O|OO&O&i", kwlist1,
PyObject *bad_keepdimarg;
Copy link
Member

Choose a reason for hiding this comment

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

Need to initialize to NULL.

@eric-wieser
Copy link
Member Author

eric-wieser commented Oct 22, 2016 •
edited
Loading

What exactly is this comment trying to achieve? Doesn't a git blame contain that information?

@eric-wieser eric-wieser force-pushed the warning-on-accumulate-keepdim branch from bcc4025 to 35736af Compare October 22, 2016 12:02
@eric-wieser
Copy link
Member Author

Updated with tests and a NULL initialization

@charris charris added this to the 1.14.0 milestone Oct 22, 2016
charris
charris reviewed Oct 22, 2016
View reviewed changes
numpy/core/tests/test_deprecations.py
"""
Deprecate the keepdims argument to np.ufunc.accumulate, which was never used or documented
"""
def test_keepdims(self):
Copy link
Member

Choose a reason for hiding this comment

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

Want this:

        with warnings.catch_warnings():
            warnings.filterwarnings('always', '', FutureWarning)
            assert_warns(FutureWarning, np.add.accumulate, [1], keepdims=True)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@eric-wieser eric-wieser force-pushed the warning-on-accumulate-keepdim branch from 35736af to 5b9e7ee Compare October 23, 2016 01:46
@charris charris merged commit 0e4de36 into numpy:master Oct 23, 2016
@charris
Copy link
Member

charris commented Oct 23, 2016

Thanks @eric-wieser .

@charris charris removed this from the 1.14.0 milestone Oct 23, 2016
@charris charris added the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Oct 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charris charris charris left review comments

Assignees
No one assigned
Labels
07 - Deprecation 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes component: numpy._core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eric-wieser @charris