MAINT: refactor ufunc iter operand flags handling by mattip · Pull Request #11580 · numpy/numpy

mattip · 2018-07-17T01:59:44Z

In order for the inner loop of matmul to use BLAS, we need to be able to require contiguous input and output memory. Additionally, if there is memory overlap between the output and the input we must not write the results to the output array until looping is finished. linalg solves this problem by copying in the inner loop, which solves the first problem but not the second. einsum and dot solve this problem by not going through PyUFunc_GenericFunction, i.e. they are not true gufuncs.

I can use a custom ufunc->type_resolver function for matmul to copy the inputs to contiguous memory and create an output array with writeback semantics, but there is nowhere to trigger resolution of those semantics after iterating. The most convenient thing would be to use iter operand flags to get the NpyIter to manage the writeback semantics for me. However, the NPY_ITER_OVERLAP_ASSUME_ELEMENTWISE defeats that strategy, it tells NpyIter to avoid creating a writeback buffer under conditions that matmul requires a writeback.

I tried one approach to solving this in PR #11381, which simply removed the NPY_ITER_OVERLAP_ASSUME_ELEMENTWISE flag from generalized ufuncs (ones that call PyUFunc_GeneralizedFunction). It seems people count on that behaviour, and so the PR was rejected.

This is an attempt at another approach, where the handling of the flags is pushed into the ufunc->type_resolver function, and I can override default behaviour in the matmul-specific one that anyhow I need in order to force input arrays to be contiguous.

In this first step, I create a generic function to set per-operand flags and use it in the three looping strategies. In later steps I will try to move the function into the generic ufunc->type_resolver.

mhvk · 2018-07-17T18:50:05Z

@mattip - I only had a glancing look so far, but I like this as a refactoring, even independently of the final goal. Was the idea to do this over multiple PRs, or over multiple commits? I think the former may be better (at least for this one), so ping me when this is no longer a WIP.

mattip · 2018-07-22T16:53:57Z

@mhvk I have gone about as far as I can go with this refactoring. I am not sure where in the code the calls to the new _ufunc_setup_flags function should be placed, I pushed the calls up to be as early as possible just to prove that the calls work and nothing else overrides the flags.

Note that the function itself changed flag handling logic so that if ufunc->op_flags[i] is set, the call will not override any of the flags. Previously output ufunc->op_flags were never read, and input flags were ored together. This is to allow the behaviour I need for matmul-as-a-ufunc (PR #11381 and the description above)

mhvk

@mattip - The changes look good but I am a bit uncomfortable with the changes to the input flags (outputs are OK, since they were never used anyway). See longer note.

cc @eric-wieser as I think it would be good to have another look.

mhvk · 2018-07-24T21:46:09Z

Shouldn't the types of op_in_flags, op_out_flags, and op_flags all be the same (i.e., all npy_uint32)? Otherwise, the assignment can be problematic.

mhvk · 2018-07-24T21:48:04Z

If op_in_flags is meant to be the default, why are we adding flags to it? I guess these are always needed? But then, shouldn't they always be added to ufunc->op_flags[i] as well? If the flags are not always needed, I think it may be better to just pass those defaults into the function.

In either case, I think the loop (both for in and out) itself should be:

op_flags[i] = ufunc->op_flags[i] ? ufunc->op_flags[i] : op_in_flags;

with then possibly the or'ing with the "standard" flags after it if they are always needed.

As I stated in the justification for the pull request: there is curently no option for user-defined flags to override the flags here, and I definitely want to exclude NPY_ITER_OVERLAP_ASSUME_ELEMENTWISE. I could also imagine a user who would want to exclude NPY_ITER_ALIGNED if their inner loop does not depend on element access being aligned. I have no strong opinion on whether the op_in_flags should be only the exceptions from the common set of flags or should be orred with a NPY_UFUNC_DEFAULT_OP_FLAGS = NPY_ITER_READONLY | NPY_ITER_ALIGNED | NPY_ITER_OVERLAP_ASSUMED_ELEMENTWISE

I guess the question is whether there are people out there relying on the flags they define being OR'd with the default ones; we ideally do not break their code.

mhvk · 2018-07-24T21:52:58Z

For the outputs, since previously their op_flags were ignored, a change in behaviour would seem OK.

codecov-io · 2018-07-25T02:01:01Z

Codecov Report

Merging #11580 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #11580   +/-   ##
=======================================
  Coverage    85.7%    85.7%           
=======================================
  Files         327      327           
  Lines       81985    81985           
=======================================
  Hits        70264    70264           
  Misses      11721    11721

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 210f07f...7480760. Read the comment docs.

mattip · 2018-07-25T17:08:02Z

Rebased off master to reduce codecov noise, squashed commits, removed possibly backward-incompatible behavior with input flags

mattip · 2018-07-25T17:09:07Z

@mhvk I left the input flag logic as it was previously

mhvk

With this, I think we're safe and can discuss the question of whether the logic for inputs should change separately.

I'm approving, but will wait a day before merging to give @eric-wieser a chance to chime in.

mattip · 2018-08-06T18:11:01Z

@eric-wieser any thoughts?

charris · 2018-08-15T16:18:27Z

lines < 80 characters.

charris · 2018-08-15T16:19:00Z

Lines too long. also below.

charris · 2018-08-15T16:19:29Z

Needs some style fixes to break up the long lines.

eric-wieser · 2018-08-21T06:32:36Z

reworked entirely

eric-wieser · 2018-08-21T06:32:44Z

This could maybe do with a docstring

eric-wieser · 2018-08-21T06:33:57Z

Are op_flags and ufunc->op_flags here ever the same pointer?

No. op_flags is the per-call copy of ufunc->op_flags, updated for the actual call arguments and variation (reduce, __call__, reduceat)

eric-wieser · 2018-08-21T06:37:44Z

linalg solves this problem by copying in the inner loop, which solves the first problem but not the second

Does this second problem have a visible effect in linalg?

eric-wieser · 2018-08-21T06:39:39Z

Docstring for this function should be updated with this new argument

mattip · 2018-08-21T10:01:57Z

Does this second problem have a visible effect in linalg?

The second problem was overlap between input and output, which means we need to use writeback semantics or another technique to prevent writing to input. As far as I can tell, the problem does not exist in linalg functions in the first place, correct me if I am wrong.

mattip · 2018-09-09T09:43:44Z

ping

mattip · 2018-10-19T12:37:23Z

@eric-wieser can we merge this?

mattip · 2018-10-27T19:51:16Z

@eric-wieser can we merge this PR? It is blocking further progress with PR #11133 matmul-as-ufunc

stefanv · 2018-10-31T20:00:08Z

Gentle ping to see if we can get this one closed down.

mattip · 2018-11-06T01:45:31Z

Merging. @eric-wieser if you have further comments please raise them in a new issue

charris added component: numpy._core 03 - Maintenance labels Jul 17, 2018

mattip changed the title ~~WIP: MAINT: refactor ufunc iter operand flags handling~~ MAINT: refactor ufunc iter operand flags handling Jul 22, 2018

mhvk reviewed Jul 24, 2018

View reviewed changes

mattip force-pushed the ufunc-flag-refactor branch from e7eb245 to 7480760 Compare July 25, 2018 17:07

mhvk approved these changes Jul 25, 2018

View reviewed changes

charris reviewed Aug 15, 2018

View reviewed changes

eric-wieser reviewed Aug 21, 2018

View reviewed changes

Comment thread numpy/core/src/umath/ufunc_object.c Outdated

eric-wieser Aug 21, 2018

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

This could maybe do with a docstring

eric-wieser reviewed Aug 21, 2018

View reviewed changes

mattip mentioned this pull request Oct 19, 2018

ENH: make matmul into a ufunc #12219

Merged

6 tasks done

eric-wieser self-assigned this Oct 19, 2018

MAINT: refactor ufunc operand flag handling and 'where mask' handling

9afa2f7

mattip force-pushed the ufunc-flag-refactor branch from 37d8f1b to 9afa2f7 Compare October 21, 2018 05:51

mattip merged commit 97df928 into numpy:master Nov 6, 2018

mattip deleted the ufunc-flag-refactor branch November 15, 2018 18:53

mattip mentioned this pull request Dec 1, 2018

ENH: Break the assumption that all ufuncs and gufuncs want is element-wise loop aliasing #11416

Closed

mattip mentioned this pull request Mar 30, 2021

BUG: where= in gufuncs is slightly broken with out= and casts #18700

Open

Conversation

mattip commented Jul 17, 2018

Uh oh!

mhvk commented Jul 17, 2018

Uh oh!

mattip commented Jul 22, 2018

Uh oh!

mhvk left a comment

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!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mattip commented Jul 25, 2018

Uh oh!

mattip commented Jul 25, 2018

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mattip commented Aug 6, 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!

charris commented Aug 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!

eric-wieser commented Aug 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattip commented Aug 21, 2018

Uh oh!

mattip commented Sep 9, 2018

Uh oh!

mattip commented Oct 19, 2018

Uh oh!

mattip commented Oct 27, 2018

Uh oh!

stefanv commented Oct 31, 2018

Uh oh!

mattip commented Nov 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

codecov-io commented Jul 25, 2018 •

edited

Loading