Skip to content
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.

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

findextrema: compute findmin and findmax in single pass #45783

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

andrewjradcliffe
Copy link
Contributor

This is a simple extension of extant findmin and findmax methods. Depending on context (cost of f; whether reduction is over dims; size of array) the speedup increase is somewhere between 1.0-1.6 (no regressions). Interestingly, I noticed but could not locate a findextrema; there is some mention of it, but nothing in Base. If it was deemed unworthy, please excuse this errant PR.

@andrewjradcliffe andrewjradcliffe changed the title Ajr/findextrema findextrema: compute findmin and findmax in single pass Jun 22, 2022
@mikmoore
Copy link
Contributor

extrema is to (minimum,maximum) as minmax is to (min,max), so perhaps findminmax is the a more consistent name here? Although there isn't an obvious notion of a distinction between findminmax and findextrema, so either would be appropriate. Wait to see what other people feel about this before you bother to change it.

@StefanKarpinski
Copy link
Member

Thanks for the PR—this does seem like desirable functionality! Anyone familiar with the reduce machinery willing to code review this?

Copy link
Member

@N5N3 N5N3 left a comment

Choose a reason for hiding this comment

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

I agree with @mikmoore on the name's consistency.
As the added funtion return findmin(), findmax() rather than extrema(), indexes.
Although findminmax! has been used for a internal method, we can rename it without breakage.

base/reducedim.jl Outdated Show resolved Hide resolved
base/reduce.jl Outdated Show resolved Hide resolved
base/reducedim.jl Outdated Show resolved Hide resolved
@andrewjradcliffe
Copy link
Contributor Author

I agree with @mikmoore on the name's consistency. As the added funtion return findmin(), findmax() rather than extrema(), indexes. Although findminmax! has been used for a internal method, we can rename it without breakage.

I went with findextrema as per the link in the OP, but perhaps opinions have changed since 2019. Whichever name we choose, we will most likely be stuck with.
My thoughts on name choice are: at first glance, findminmax seems most natural, as it confers the notion of find_min_and_max. Alas, minmax looks quite similar to minimax, which is an entirely different concept. Alternatively, findextrema, while arguably uglier, is unambiguous in relation to extrema.

@KristofferC
Copy link
Member

Can you add something to the NEWS.md file about this?

andrewjradcliffe added a commit to andrewjradcliffe/julia that referenced this pull request Nov 4, 2022
At the request of @KristofferC , the relevant news update.
A separate PR is perhaps not ideal, but, given that NEWS.md moves
at a different pace than JuliaLang#45783, it seemed to make sense.
This is a simple extension of extant `findmin` and `findmax` methods.
Depending on context (cost of `f`; whether reduction is over dims;
size of array) the speedup increase is somewhere between 1.0-1.6 (no regressions).
Interestingly, I noticed but could not locate a `findextrema`; there
is some [mention](JuliaLang#7327) of it,
but nothing in Base. If it was deemed unworthy, please excuse this
errant PR.
Embarrassingly, the original `findextrema` over all elements did not
guarantee a single evaluation of `f` per iteration.
@brenhinkeller brenhinkeller added the search & find The find* family of functions label Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
search & find The find* family of functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants