Implement argmin argmax

[phungleson]

Hey mates, this PR currently has a rough impl for argmin for discussions.

Please have some comments. Once everyone is happy with the implementation, I will add necessary docs, tests.. and other methods.

[munckymagik]

Hi @phungleson thanks for the contribution!

I think it was a great idea to raise this PR and get a conversation started 👍. I'm a volunteer. I am happy to keep an eye out for comments and changes, review/discuss ideas and generally cheer you on.

It certainly looks like this code would work, but I agree we need to get to a point where we are all happy it's the right implementation.

I suggest the next step in achieving that is to wrap some tests around it so we can experiment with the implementation without risk of breaking things. May not need to be deep coverage at this stage, but it's up to you. I would think about relevant ndarray shapes, types and error cases.

From quick review it looks like its run-time complexity is O(2n) big-oh of 2n. If that's the case, then my instinct is we could improve on that.

Have you had a look for ideas from existing implementations in other technologies? For example numpy and Julia - how do they implement this? Are there any special algorithms people use that are better than the intuitive approach?

Thanks. I hope this helps.

[phungleson]

Hey @munckymagik thanks for the comment! :) I have added a few tests and argmax

For numpy.argmin, it shows that it delegates to some sort of getattr

https://github.com/numpy/numpy/blob/v1.10.1/numpy/core/fromnumeric.py#L967
https://github.com/numpy/numpy/blob/v1.10.1/numpy/core/fromnumeric.py#L43-L53

For Julia, it seems like it just get the first index of min and returns that value (not a vec).

https://github.com/JuliaLang/julia/blob/80516ca20297a67b996caa08c38786332379b6a5/base/array.jl#L2159
https://github.com/JuliaLang/julia/blob/80516ca20297a67b996caa08c38786332379b6a5/base/array.jl#L2096-L2115

Perhaps we can do it in a way similar to Julia but we can return a vec of indices of min value. What do you think?

[LukeMathWalker]

Thanks for jumping in @munckymagik! The issues and PRs of this repo are getting crowded, that's super cool 🚀

I guess the main choice we have to make concerns the number of outputs to be returned: if there is more than one minimum/maximum value, do we return just the indices for the first minimum/maximum value we encounter or do we want to return them all?

If we align with Julia/NumPy, returning just the first one, then we should be able to get complexity down to O(n) - you don't need to "precompute" the minimum using the min method 👍 It should also be possible to have O(n) complexity returning all mins/maxs avoiding the pre-computation while timely emptying the array you are using to accumulate values 🤔

[phungleson]

Cool thanks, I have changed to return a single value. As the idea of this lib is to be closer to numpy, perhaps we can keep this?

Let me know if returning a vec is desirable, it shouldn't be too hard.

If this looks ok, I will add argmin_skipnan argmax_skipnan as suggested.

[LukeMathWalker]

I left some comments for polishing, but the functionality is there 💯 @phungleson

[phungleson]

@LukeMathWalker Hey mate, I have made the changes, let me know if anything else is still missing.

I probably will add argmin_skipnan argmax_skipnan in a separate PR to keep this one short.

[LukeMathWalker]

I agree with small PRs - feel free to work the skipnan versions on a separate one.
This one looks good to me - I'll squash and merge.