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

WIP: max/min -> maxof/minof #2419

Closed
wants to merge 2 commits into from

Conversation

blakejohnson
Copy link
Contributor

Implements the renaming of the binary max/min functions to maxof and minof as discussed in #2265. Now, if max or min are given a second argument, it is interpreted as the dimension or region over which to take the max or min.

Bye, max(a, (), 1)!

Note: this required updates in a lot of files. It passes all tests on my machine, but additional testing might be wise before merging.

@blakejohnson
Copy link
Contributor Author

Whoops, forgot about extras/. I'll fix that in a minute.

@StefanKarpinski
Copy link
Member

Woah. This is a surprisingly large change. That's not necessarily bad but it definitely needs to sit for a bit and collect some commentary.

@JeffBezanson
Copy link
Member

The size of this change reflects that max is a very standard and commonly-used function as is. This is revealing.

@blakejohnson
Copy link
Contributor Author

Well, we only need max/min on a dimension for non-scalar arguments. So, the other option is to make max(array, scalar) always mean max along a dimension, rather than max(append(vector, scalar))

@tshort
Copy link
Contributor

tshort commented Feb 27, 2013

I prefer that max([0,2,3], 2) gives [2,2,3] and leave operation along a dimension to another function or to Jeff's upgraded sweep_dim or whatever it would be called.

@StefanKarpinski
Copy link
Member

See this is where it gets kind of ugly. Since max and min have this problem and they're in some sense related to mean and median (as statistics one can take of a set of values), it gets kind of weird that some statistics can take a dim argument and some can't. Using a dim keyword would solve this, of course.

@StefanKarpinski
Copy link
Member

Unfortunately, there's a reason why this issue hasn't been satisfactorily resolved yet :-\

@blakejohnson
Copy link
Contributor Author

@tshort I prefer that functionality in a statement like map(x->max(x,2), [0,2,3]).

@ViralBShah
Copy link
Member

The code is actually a lot more readable now.

@blakejohnson
Copy link
Contributor Author

@tshort Having ruminated on this for a day, I see now why your max([0,2,3], 2) => [2,2,3] is intuitive in some sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants