It works, but it is not very valuable, right? I mean, main objection was that findmax(f, itr) should work with any iterator, even those which has no index.
With proposed change key feature of the current version of findmax(f, itr) is nullified (triplet will work only for arrays) and the only addition is v[ix] which is not that important. I mean, it literally just remove one line from
val, ix = findmax(f, A)
domain = A[ix]
Also, three values returned as a tuple is very confusing I already do not remember which one is index, which is A[ix] and f(A[ix]).
This is a different issue (but stil, a gotcha). In Julia vectors, pairs, and other iterables are ranked lexicographically. so < is defined for pairs, but of course sqrt is not.
This is what I would suggest also. Since we have a contract for findmax(v) that we cannot change till 2.0 release (like it or not) I think we should follow the same contract in findmax(f, v) and a function that works for any iterable needs a new name.
I wrote the PR that added these methods (with help from tkf and others). The implemented behaviour is as intended.
We acknowledged in the PR that findmax(identity, itr) != findmax(itr) is a bit confusing, but the docstrings do accurately describe the behaviour. A previous iteration of the docstrings also gave the rationale for findmax(xs) being different: when xs is an object that has a domain and codomain then you don’t need to give a function.
Personally, I think the new methods make a lot of sense and findmax([1, 2, 3]) also makes sense to me, but y’know, if everyone hates it we can do something else.
Also, the behaviour with findmax(Dict(2 => 3, 4 => 5)) is the same in 1.6 and 1.7. Any breaking change in behaviour of findmax(itr) between versions is unintentional (please let me know/open an issue). It is expected that the 1.7 implementation works with some values that would error in 1.6, but that’s intentional.
What exactly do you mean here?
If this is about arrays and other indexable collections, then it is not really expected to treat them as functions. Unlike matlab, julia doesn’t consider indexing arrays and calling functions as operations of the same kind.
I believe that a wider context was not evaluated enough in that design.
It would be a different story if this was the only case of a reduction that takes an array and a function. However, there are lots of these in julia already, and they all do roughly r(f, A) == r(map(f, A)).
Discussion here, and numerous questions in slack (I myself saw ~4, and this is for nightlies!), are additional evidence for this being confusing.
While the rationale behind the decision makes sense. From the point of view of the user it’s still very confusing. Most functions that work similarly to findmax(itr) i.e mean, sum work the same when calling sum(identity,itr) so for consistency I would argue that findmax(identity,itr) == findmax(itr). So maybe creating two separate functions might be the best solution.
This is also my point. That is why I have said that it is an “unfortunate case” above. I have spent many days on working out similar dilemmas in DataFrames.jl. Let me give an example (just to show I understand the pain). We had join function in DataFrames.jl that was well documented. However, it did something else than join in Julia Base. There was really a very low risk of confusion. Still we decided to remove it and create new function names (if I recall correctly @kristoffer.carlsson suggested this).
I know that you have spent a lot of time discussing pros and cons of findmax(fun, itr) and it is a reasonable design. What I (and probably other users here) want to voice that we are afraid that it will be hard to learn/use/teach this design even if it makes sense and is a best possible solution from an advanced user’s perspective. Of course Julia core dev team probably should weigh pros and cons and make a decision.
I think the new behavior seems a lot more like a proper argmax – you get both the max f(x) and x. To me, this is how an argmax works. That being said, that is NOT what I would expect a findmax function to do. In that case, I’d expect the maximum f(x) and its index. So, my thoughts would be to drop the argmax(x) methods and replace them with this new behavior, and leave findmax(f,a) as others have already said.
It would indeed be great to have concepts of mathematical argmax and indmax (currently called argmax) separated. However, what you suggest is breaking, and cannot happen in julia 1.x.
Yeah, I do get how it’s different from e.g. sum(f, A), and that was the context we were evaluating.
It is obviously legit to be querying this design decision, that’s why it’s in nightly, after all.
@aplavin, yes the intended interpretation is that indexable collections are maps from keys to values, in the same way that a function maps from inputs to outputs.
It’s true that that’s not how we think of collections in Julia, but it is consistent with how functions and collections are sometimes described in CS and maths and it gives one consistent meaning to this family of functions (they always do something with the domain and codomain, and if you don’t provide a function then the domain and codomain are the implicit ones for that object). You can then imagine defining your own methods for e.g. findmax(x::DifferentialEquationModel) or whatever.
It might be better if argmax(xs) didn’t have its current definition, but we just have to live with it.
Hm. In that interpretation, two argument findmax/findmin over dicts is broken then, since it (by that interpretation) should iterate over keys, and not the key-value tuple the nightly version currently does (unlike the single argument version, which does the correct thing).
Is there any other place in julia where arrays and functions are treated equivalently? I see that they are considered fundamentally different everywhere (and this is a good thing!):
julia> A = [1, 10, 100];
julia> A[2]
10
julia> A(2)
ERROR: MethodError: objects of type Vector{Int64} are not callable
julia> map(x -> A[x], 1:3)
3-element Vector{Int64}:
1
10
100
julia> map(A, 1:3)
ERROR: MethodError: objects of type Vector{Int64} are not callable
julia> sum(x -> A[x], 1:3)
111
julia> sum(A, 1:3)
ERROR: MethodError: objects of type Vector{Int64} are not callable
# etc
If we assume the interpretation @ColinCaine gave, then findmax(f, ::Dict) implies the domain is pairs just as in findmax(f, ::AbstractArray), the domain is values of the array.
The heart of the problem is that under that interpretation, f is implicitly i -> A[i] and k -> d[k] in the array and dict variants for a single argument. Using identity is not equivalent in this interpretation.
I just wanted to clarify; I don’t have a horse in this race (yet).
I know that this is the difference. I think assuming tuples as the domain is the wrong choice here, that’s all, since Dicts are very much “indexable” in the sense that they have getindex defined. They’re just not a rectangular data store.
I guess what I’m saying is that I think findmax and friends should (for indexable collections) call keys(coll), index with them (in order returned by keys) into the collection, pass whatever they get to the supplied f and use the result for max/minimality, returning the tuple (f(col[idx]), idx). I don’t think this is solvable for indexable and non-indexable collections/iterables in one swoop.
Cool, I think we both understand what the other wants and we just disagree on what the method should do?
In case there’s any doubt (and because I wrote this before you wrote your next reply):
The distinction we made in the original PR is that we only imply the domain and codomain when a function has not been given.
If a function has been given then the domain is the iterable and the codomain is the outputs of the function for the values of the iterable.
This gives consistent behaviour for Dicts, Arrays and any other indexable collection, and gives a useful result for non-indexable collections when you provide f. And I’ll emphasise that I think findmax on non-indexable collections is a useful thing to have.
I do not think, that anyone is saying that nightly findmax is useless or bad some other way. It’s just inconsistent with other Julia ecosystem. Name it findmaxvalue or something like this and it will be used in many applications, no doubt.
I already do not remember which one is index, which is