8000 ENH/WIP: add a max_abs ufunc by ewmoore · Pull Request #5509 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

ENH/WIP: add a max_abs ufunc #5509

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
Closed
wants to merge 1 commit into from
Closed

ENH/WIP: add a max_abs ufunc #5509

wants to merge 1 commit into from

Conversation

ewmoore
Copy link
Contributor
@ewmoore ewmoore commented Jan 27, 2015

This is a follow on to our discussion in #5420, about adding sum_abs and max_abs ufuncs. Testing here show this version to be almost as fast as a raw maximum.reduce for float64 and float32. In both the SIMD loop and the BINARY_REDUCE_LOOP paths.

Still needs tests.

@argriffing
Copy link
Contributor

Thanks for writing this! I guess the new PyUFunc_MaxAbsTypeResolver could also be used for sum_abs type resolution later?

I see that the USE_CHROOT=1 ARCH=i386 DIST=trusty PYTHON=3.4 Travis config and the USE_BENTO=1 config fail the double and single precision vector norm tests, but I couldn't track down the source of the problem.

juliantaylor
juliantaylor reviewed Jan 27, 2015
View reviewed changes
numpy/core/src/umath/funcs.inc.src
int cmp;

absi1 = PyNumber_Absolute(i1);
absi2 = PyNumber_Absolute(i2);
Copy link
Contributor

Choose a reason for hiding this comment

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

these can probably raise exceptions and return NULL which needs handling (and a test)

@juliantaylor
Copy link
Contributor

shouldn't it be named abs_max, the absolute is applied first, similar for other functions of this type like square sum

@argriffing
Copy link
Contributor

shouldn't it be named abs_max

I'd thought that maxabs is a thing, kind of like how logsumexp is a thing. One example is https://software.intel.com/en-us/node/502168. But I may be wrong.

@juliantaylor
Copy link
Contributor

hm maximum of absolute, sum of squares, I guess it makes sense. If other libraries use this ordering its probably best to not deviate

@juliantaylor
Copy link
Contributor

instead of adding lots of new functions it would be kind of nice to have a sort of preproces argument for ufuncs that is another ufunc. Then you could e.g. do:
np.max(a, preprocess=np.abs) or possibly even recursive chains like np.max(a, preprocess=partial(np.abs, preprocess=partial(np.power, b=3)))

kind of explicit unreadable lazy evaluation, on second though probably not so such a great idea

@argriffing
Copy link
Contributor

instead of adding lots of new functions it would be kind of nice to have a sort of preproces argument for ufuncs that is another ufunc.

This reminds me of the discussion near #5218 (comment).

@njsmith
Copy link
Member
njsmith commented Jan 27, 2015

I think you're about 20% of the way to reinventing numexpr?

Which is not a bad thing, a version of numexpr that worked with the regular
ufunc machinery instead of reimplementing it all would be awesome.

On Tue, Jan 27, 2015 at 8:52 PM, Julian Taylor notifications@github.com
wrote:

instead of adding lots of new functions it would be kind of nice to have a
sort of preproces argument for ufuncs that is another ufunc. Then you could
e.g. do:
np.max(a, preprocess=np.abs) or possibly even recursive chains like np.max(a,
preprocess=partial(np.abs, preprocess=np.partial(np.power, 3)))

kind of explicit unreadable lazy evaluation, on second though probably not
so such a great idea

—
Reply to this email directly or view it on GitHub
#5509 (comment).

Nathaniel J. Smith
Postdoctoral researcher - Informatics - University of Edinburgh
http://vorpus.org

@njsmith
Copy link
Member
njsmith commented Jan 27, 2015

Basically we'd need two things:

  • a way to run an arbitrary ufunc expression graph over cache-sized chunks
    (not terribly hard in principle, I think -- define a ufunc loop that
    recurses over some data structure calling other ufunc loops?)

  • some user-friendly way to write down these expression graphs. This is
    just a UI problem though and should be decoupled from the above -- instead
    of awkward kwargs like prefilter= we there are lots of nice options
    available e.g.

    a = delay(a) # returns a "delay" object with trivial graph
    ma = np.maximum.reduce(np.abs(a)) # returns a "delay" object with
    nontrivial graph
    ma.force() # evaluates the graph and returns an ndarray object

numpy_ufunc gets us part of the way there...

Of course one could get a minimum-viable-product by starting with
supporting relatively restricted expression graphs like pure unary nesting
and then improve from there.

On Tue, Jan 27, 2015 at 9:00 PM, Nathaniel Smith njs@pobox.com wrote:

I think you're about 20% of the way to reinventing numexpr?

Which is not a bad thing, a version of numexpr that worked with the
regular ufunc machinery instead of reimplementing it all would be awesome.

On Tue, Jan 27, 2015 at 8:52 PM, Julian Taylor notifications@github.com
wrote:

instead of adding lots of new functions it would be kind of nice to have
a sort of preproces argument for ufuncs that is another ufunc. Then you
could e.g. do:
np.max(a, preprocess=np.abs) or possibly even recursive chains like np.max(a,
preprocess=partial(np.abs, preprocess=np.partial(np.power, 3)))

kind of explicit unreadable lazy evaluation, on second though probably
not so such a great idea

—
Reply to this email directly or view it on GitHub
#5509 (comment).

Nathaniel J. Smith
Postdoctoral researcher - Informatics - University of Edinburgh
http://vorpus.org

Nathaniel J. Smith
Postdoctoral researcher - Informatics - University of Edinburgh
http://vorpus.org

@ewmoore
ENH: add a max_abs ufunc
f10300c 
This function computes maximum(absolute(x), absolute(y)), which
when used as a reduction is the inf-norm of a vector.  Providing
a single call ufunc for this operation is 2x-3x faster than the
composite operation.  For large array sizes, this also eliminates
a full sized temporary.  On x86_64 this operation is similar in
speed to maximum for contiguous reductions.
@ewmoore
Copy link
Contributor Author
ewmoore commented Feb 25, 2015

I've removed the code that depends on the float representation from loops.c.src. However, This is no longer faster than just calling abs(arr).max() when the BINARY_LOOP path is taken. It looks to me as though gcc must generate about exactly the code I had, where I was masking out the sign bit instead of performing a comparison to find the absolute value if I use fabs directly. Calling npy_fabs obviously doesn't cut it since gcc doesn't know anything about it.

This version is about a factor of 2 slower for the non-contiguous case. I'm not sure what kind, if any, code changes can regain that speed in a fully portable way. The script I've been using to benchmark things is here: https://gist.github.com/ewmoore/5f34c148b00ed461057d

Although I think allowing arbitrary combinations of ufuncs would be nice and might make a good gsoc project I'm not going to have time to do that. So if we'd rather build that instead, lets close this. Its also possible to only expose this function (and a hypothetical sum_abs) via linalg.norm, perhaps that represents a sort of middle road. That being said, it seems to me that we don't want to make this slower, which this actually does right now.

@charris
Copy link
Member
charris commented Mar 22, 2015

Rejigger tests by closing and opening.

@charris charris closed this Mar 22, 2015
@charris charris reopened this Mar 22, 2015
@argriffing
Copy link
Contributor

Although I think allowing arbitrary combinations of ufuncs would be nice and might make a good gsoc project I'm not going to have time to do that.

I think it's reasonable to not do everything in the maximum amount of generality possible. For example there's a short list of functions in https://software.intel.com/en-us/node/502164. Personally I am most interested in inf-norm and 1-norm which could be implemented as max_abs and sum_abs ufuncs.

So if we'd rather build that instead, lets close this.

In my opinion there could be value in not waiting for arbitrary combinations of ufuncs.

Its also possible to only expose this function (and a hypothetical sum_abs) via linalg.norm, perhaps that represents a sort of middle road.

This seems like a good idea to me.

@ewmoore ewmoore closed this Feb 1, 2016
@charris
Copy link
Member
charris commented Feb 1, 2016

@ewmoore How come?

@ewmoore
Copy link
Contributor Author
ewmoore commented Feb 1, 2016

Mostly because I don't intend to work on this further right now. I think
its a fine idea though.

On Mon, Feb 1, 2016 at 11:25 AM, Charles Harris notifications@github.com
wrote:

@ewmoore https://github.com/ewmoore How come?

—
Reply to this email directly or view it on GitHub
#5509 (comment).

@ewmoore
Copy link
Contributor Author
ewmoore commented Feb 1, 2016

I'm very sympathetic to the idea that having all of these
dead/stalled/abandoned PRs around isn't great for the project. Scipy has
the same problem.

On Mon, Feb 1, 2016 at 11:57 AM, Eric Moore ewm@redtetrahedron.org wrote:

Mostly because I don't intend to work on this further right now. I think
its a fine idea though.

On Mon, Feb 1, 2016 at 11:25 AM, Charles Harris notifications@github.com
wrote:

@ewmoore https://github.com/ewmoore How come?

—
Reply to this email directly or view it on GitHub
#5509 (comment).

@charris charris added the 52 - Inactive Pending author response label Feb 1, 2016
@jakirkham jakirkham mentioned this pull request Aug 17, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
01 - Enhancement 52 - Inactive Pending author response component: numpy._core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ewmoore @argriffing @juliantaylor @njsmith @charris @rgommers
0