ENH: Add keepdims argument for generalized ufuncs. by mhvk · Pull Request #11098 · numpy/numpy

mhvk · 2018-05-14T16:58:39Z

~~This is a single commit from #11018, to try to find the reason for the travis build failure.~~

EDIT: this is a split-off of #11018, just implementing keepdims, to keep discussion more clearly separate.

eric-wieser · 2018-05-14T17:39:15Z

This function should return an error code (0, -1) not a boolean (1, 0), since it's called _check_support not _is_supported (and sets an exception)

mhvk · 2018-05-14T19:41:08Z

@eric-wieser - OK, this now contains also the documentation for keepdims (as a second commit), and I corrected the usage of _check_keepdims_support.

mhvk · 2018-05-14T19:50:59Z

Darn, this should be axes here, of course. Will correct together with any other remaining comments.

mhvk · 2018-05-15T17:10:34Z

@eric-wieser - you looked at this quite a bit already, do you think it is OK? If so, I can do the follow-up for just axis.

eric-wieser · 2018-05-15T17:35:19Z

nit: would probably combine these two lines - we do that pretty frequently

As in if (retval = ... < 0)? I don't see any example of that in ufunc_object, and personally don't like it much. Anyway, will await for the more thorough look.

I mean:

if (_check_keepdims_support(ufunc) < 0) { goto fail; }

But fail needs to have retval set...

What am I missing here? Is retval sometimes not -1 on failure?

It left at 0 by the get_ufunc_arguments statement above. There is a debug statement in fail that prints retval, but to be honest I'm not sure it ever has a value other than -1 for failure. Probably would make sense to clean that all up, but perhaps in another PR...

Yep, no need to change this now - just looked a little worrying

eric-wieser · 2018-05-15T17:40:02Z

Looks pretty reasonable - I'll take a more thorough look later. I can't say I fully understand the gufunc internals yet.

eric-wieser · 2018-05-18T07:26:49Z

minor nit: I'd find this easier to read with an assignment all_num_dims = ufunc->core_num_dims[0] or similar outside the loop - too easy to read this as an elementwise copy. But maybe I'm just tired

eric-wieser · 2018-05-18T07:28:49Z

Possibly clearer with an if, because then that gives you somewhere to add a comment explaining why it is 1,

I expanded the comment above the loop (which is not very useful as is...)

eric-wieser · 2018-05-18T07:31:05Z

Unrelated to this PR: I'm struggling to understand what j is, and why its state is kept between outputs

I agree, I turn from thinking it clear to having no clue almost every time I look at this... Mostly because I only partially understand how the iterator works. But this would be a good place to comment it more heavily. For another PR...

eric-wieser

Only minor things to complain about here - generally looks like an unobtrusive change

The argument can only be used if all inputs share the same number of core dimension, and no output has any core dimensions.

Both in the general documentation and in the release notes. For the latter, also include a description of ``axes``, which was missing so far.

mhvk · 2018-05-18T13:47:52Z

@eric-wieser - OK, I made the last changes. Once merged, I'll go ahead and rebase the axis implementation on top, so we can look at that again (or, rather, think about how to deal with it for __array_ufunc__).

eric-wieser · 2018-05-18T17:10:58Z

+    correctly against the inputs. This option can only be used for generalized
+    ufuncs that operate on inputs that all have the same number of core
+    dimensions and with outputs that have no core dimensions , i.e., with
+    signatures like ``(i),(i)->()`` or ``(m,m)->()``. If used, the location of


Do we also want to support (i),(i)->(),(i)?

I thought about this when I was revisiting the text, and wasn't sure. In principle, there is no reason not too, except I then have to be more careful with the shape of the output... What do you think?

I don't think we need to decide in this PR - we can add that feature later if we can justify it. I think we should open an issue suggesting it as an enhancement, and merge this PR without any further changes.

OK, agreed; see #11118. I'll merge this one then.

eric-wieser reviewed May 14, 2018

View reviewed changes

mhvk force-pushed the gufunc-keepdims branch from 379eda8 to b589404 Compare May 14, 2018 19:37

mhvk added 01 - Enhancement component: numpy._core labels May 14, 2018

mhvk added this to the 1.15.0 release milestone May 14, 2018

mhvk changed the title ~~DO NOT MERGE Add keepdims argument for generalized ufuncs.~~ ENH: Add keepdims argument for generalized ufuncs. May 14, 2018

mhvk commented May 14, 2018

View reviewed changes

mhvk force-pushed the gufunc-keepdims branch from b589404 to 9579552 Compare May 15, 2018 17:09

eric-wieser reviewed May 15, 2018

View reviewed changes

eric-wieser reviewed May 18, 2018

View reviewed changes

eric-wieser approved these changes May 18, 2018

View reviewed changes

mhvk added 2 commits May 18, 2018 09:46

ENH: Add keepdims argument for generalized ufuncs.

c29e267

The argument can only be used if all inputs share the same number of core dimension, and no output has any core dimensions.

DOC: Describe new keepdims arguments (and axes).

8ccabd6

Both in the general documentation and in the release notes. For the latter, also include a description of ``axes``, which was missing so far.

mhvk force-pushed the gufunc-keepdims branch from 9579552 to 8ccabd6 Compare May 18, 2018 13:46

eric-wieser reviewed May 18, 2018

View reviewed changes

mhvk mentioned this pull request May 18, 2018

Possible generalization of gufunc keepdims for multiple outputs #11118

Open

mhvk merged commit 15f19c4 into numpy:master May 18, 2018

mhvk deleted the gufunc-keepdims branch May 18, 2018 20:12

This was referenced May 28, 2018

MAINT: avoid setting non-existing gufunc strides for keepdims=True. #11176

Merged

Gufunc helper for sizes, flags and num_dims? #11228

Open

Conversation

mhvk commented May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eric-wieser May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented May 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk commented May 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mhvk commented May 14, 2018 •

edited

Loading

eric-wieser May 14, 2018 •

edited

Loading

eric-wieser May 16, 2018 •

edited

Loading

eric-wieser May 16, 2018 •

edited

Loading