Idxmin & idxmax

[facorazza]

These are two procedures that return the index of the corresponding minimum and maximum value.

A cleaner version of #12976

[SolitudeSF]

this doesnt work correctly.

let a: array[-1..1, int] = [0, 1, 2]
echo a[a.idxmin] == 0

this fails to compile

type Count = enum First, Second, Third
let a: array[Count, int] = [0, 1, 2]
echo a[a.idxmin] == 0

[juancarlospaco]

result = 0 is not needed.

[zedeus]

Isn't this equivalent to just doing s.find(max(s)) (and min)?

[facorazza]

Isn't this equivalent to just doing s.find(max(s)) (and min)?

It is however find() and max() require 2 loops over the sequence whereas idxmax() needs only one so the latter is faster

[facorazza]

result = 0 is not needed.

Thank you

[facorazza]

this doesnt work correctly.

let a: array[-1..1, int] = [0, 1, 2]
echo a[a.idxmin] == 0

this fails to compile

type Count = enum First, Second, Third
let a: array[Count, int] = [0, 1, 2]
echo a[a.idxmin] == 0

The reason why your first example doesn't work is because the indices of OpenArrays go from 0 to N-1. Perhaps it would be better not to use OpenArrays for this proc but I will have to look into it.

Regarding the second example I'm not sure how to easily fix it at the moment.

[Clyybber]

@imperator26 Simply do for i in 0..<s.len or for i in 0..s.high

[facorazza]

@Imperator26 Simply do for i in 0..<s.len or for i in 0..s.high

What is this related to?

[Clyybber]

@Imperator26 Sorry, it was a dumb suggestion, I guess you need seperate procs for array and seq to fix the issues this approach has

[Araq]

The return type must be int in order to avoid bad effects like:

var m = idxmin(myarray) # returns 0; m is inferred to be Natural
dec m # dies at runtime

And yes, this hints at a general issue with Natural that I hope to sort out.

[Araq]

The loops should start at 0! Ping @Imperator26

[facorazza]

The loops should start at 0! Ping @Imperator26

If it's not working on your system it's because you've set int as a return type.
I'm using Naturals

The return type must be int in order to avoid bad effects like:

var m = idxmin(myarray) # returns 0; m is inferred to be Natural
dec m # dies at runtime

And yes, this hints at a general issue with Natural that I hope to sort out.

I think the result should be Natural since it's an index of an array and negative index are not supported by OpenArrays. If you want to do dec index you should expect it to die and either transform it to an int yourself or check if you're going out of bounds.

[Araq]

Sorry, misread the code. Starting with 1 is correct. The return type must still be int though.

I think the result should be Natural since it's an index of an array and negative index are not supported by OpenArrays. If you want to do dec index you should expect it to die and either transform it to and int yourself or check if you're going out of bounds.

That's all true but len returns int too, so for consistency please use int.

[planetis-m]

result = low(a)
for i in low(a)+1..high(a):

is more correct

[facorazza]

result = low(a)
for i in low(a)+1..high(a):

is more correct

low() of an OpenArray will always return 0 so there is no need for that

[planetis-m]

Try this:

type
   MyArr = array[-1..1, int]

var arr: MyArr
arr[-1] = -1

echo idxmin(arr)

Can this be renamed to minIdx or even minIndex so its easier to autocomplete and understand the meaning of the function?

[facorazza]

Try this:

type
   MyArr = array[-1..1, int]

var arr: MyArr
arr[-1] = -1

echo idxmin(arr)

Can this be renamed to minIdx or even minIndex so its easier to autocomplete and understand the meaning of the function?

Could you please read the code and the entire discussion? I'm getting tired of answering these kind of comments

[facorazza]

I just read it and you are still wrong. (so its an invariant)

That is very constructive...

Please read this comment

[planetis-m]

Saying you are tired when discussing is also very constructive I guess, so if it doesn't work correctly don't add this function. Use whats more correct - make a bug - and wait till its fixed.

[facorazza]

Saying you are tired when discussing is also very constructive I guess, so if it doesn't work correctly don't add this function.

It does work keeping in mind the limitations of OpenArrays. You're welcome to contribute BTW

[planetis-m]

My conclusion is this: manual says openarrays always start at 0, this function returns wrong result at some cases, please don't add this to stdlib. Have a nice one and bye

[facorazza]

My conclusion is this: manual says openarrays always start at 0, this function returns wrong result at some cases, please don't add this to stdlib. Have a nice one and bye

If you read the whole discussion you would realise that those cases are still to be implemented, but I guess reading is an option for you. Please don't comment here again unless you've got some constructive suggestions to give us.

[alehander92]

@Imperator26 i kinda agree with your technical arguments, but your defense does sound somehwhat condescending, everybody is free to comment here.

[planetis-m]

Ok, really sorry if I sounded too condemning of including this function but I don't think it is something that can be fixed (judging by the fact that open array are just pointer+len pairs.

[facorazza]

@Imperator26 i kinda agree with your technical arguments, but your defense does sound somehwhat condescending, everybody is free to comment here.

What I was trying to say is that one should read before asking the same thing twice

[alehander92]

ok, but they are valid unresolved questions? let's solve them:
you can overload those for array[<enum>..] so this case can work
you can return a.len - when n is negative for -1

[alehander92]

sorry, the second one doesnt make sense, but then it should be possible to just overload for array in principle and introspect on its element type

[planetis-m]

@alehander92 thanks for rescuing the discussion you are really my hero <3

[planetis-m]

@Imperator26 as you can see my friend the nim community is the friendliest on github so stick around and we'll build a lot of cool software using nim.

[alehander92]

sorry, i was just trying to make sense of it, i hope @Imperator26 doesn't mind

proc idxmin*[T, U](s: array[T, U]): T =
  for i in low(s)..high(s):
    if s[i] < s[result]: result = i

seems to work for those examples to me and being consistent with @Imperator26 's logic to return valid index types (and it should be possible to separately overload e.g. range cases to always return int, but i do hit some surprises in generics)

[alehander92]

e.g. this works

proc idxmin*[T; U](s: array[T, U]): auto =
  var res = low(s)
  for i in low(s).succ..high(s):
    if s[i] < s[res]:
      res = i
  when T isnot enum: # we can use a concept checking for .int here .. 
    result = res.int
  else:
    result = res

if Araq wants int for anything with int-like type except enum (which does seem consistent with len)

EDIT: remove T: range
EDIT: use low and succ

[alehander92]

( @Imperator26 notice you can use succ as a more general low + 1 it seems ?)

[facorazza]

e.g. this works

proc idxmin*[T; U](s: array[T, U]): auto =
  var res = low(s)
  for i in low(s).succ..high(s):
    if s[i] < s[res]:
      res = i
  when T isnot enum: # we can use a concept checking for .int here .. 
    result = res.int
  else:
    result = res

if Araq wants int for anything with int-like type except enum (which does seem consistent with len)

EDIT: remove T: range
EDIT: use low and succ

Thank you for this I'll have a closer look later

[Araq]

The array[Enum] case sounds like a "won't fix" to me, every proc that takes an openArray in Nim is not concerned with this special case. Maybe we can slightly change the language that only arrays starting with 0 are actually convertible to openArray.

[alehander92]

but why, if we can just support it with an additional overload: we need some kind of policy for those enum/distinct/range usecases and many similar api-s and openArray doesn't have to apply to them still

[Araq]

It's high mental overhead writing and reading the code that can deal with "-11 based indexing".

[facorazza]

Try this:

type
   MyArr = array[-1..1, int]

var arr: MyArr
arr[-1] = -1

echo idxmin(arr)

Can this be renamed to minIdx or even minIndex so its easier to autocomplete and understand the meaning of the function?

this works now and I'm considering the name change

[facorazza]

Any thoughts on the names of the procedures?

[andreaferretti]

Any thoughts on the names of the procedures?

I like minIndex and maxIndex best. For what it's worth, it's what I use in neo

[facorazza]

Any thoughts on the names of the procedures?

I like minIndex and maxIndex best. For what it's worth, it's what I use in neo

Yes I also think they're clearer names

[Araq]

Narimiran took over, thanks for your work!

[facorazza]

Narimiran took over, thanks for your work!

The merged code does not address the use cases described here while the code in this pr does. Is there any reason to why the accepted code was redacted?

[Araq]

1. I doubt the code really was correct for e.g. array[-3..10, T].
2. The rest of the stdlib uses openArray and doesn't bother with weird arrays either. We cannot accept the resulting factor of 2 in API surface and the maintainability effort it implies.

[facorazza]

1. I doubt the code really was correct for e.g. array[-3..10, T].
2. The rest of the stdlib uses openArray and doesn't bother with weird arrays either. We cannot accept the resulting factor of 2 in API surface and the maintainability effort it implies.

I'm not sure about the first point but I can understand the reasons in the second one. Is there any chance for snippets like these ones to be gathered in an external library?

[Araq]

Sure, feel free to create Nimble packages.