Thanks Konrad for picking this up 👍

We should think through what behavior we want. The current solution assumes an array, and picks a random element from the array. In mathjs you often have say a 2d matrix or array, and I think it makes more sense though to pick an element from the matrix instead of returning a row of the matrix. For example:

// create a 2x2 array
const array = [
  [1, 2], 
  [3, 4]
]
math.pickRandom(array) 
// currently returns a "row": [1, 2] or [3, 4]
// I would expect it to return a value 1, 2, 3, or 4

I also noticed an unrelated bug that is already in the existing implementation:

    if (length === 0) {
      return []
    } else if (number >= length) {
      return number > 1 ? possibles : possibles[0]
    }

This causes input like pickRandom([1,2,3], 10) to always return [1, 2, 3] without the result being random nor 10 values being returned. Maybe you can also have a look into this?

@josdejong I tried to implement the behavior described in #1974 which was to pick any element from the array. So that's what I did, but it sure makes sense to behave differently in case of matrices.

Yes indeed. I think this should be easy to achieve, by doing possibles = flattenArray(possibles.valueOf())

I used flatten, but now in some tests I'm getting AssertionError [ERR_ASSERTION]: Input objects identical but not reference equal and it makes sense for me that the reference shouldn't be equal for example here should return the given array if the given number is equal its length, so probably tests are wrong.

It could be that we need to change some tests.

@josdejong
Regarding

This causes input like pickRandom([1,2,3], 10) to always return [1, 2, 3] without the result being random nor 10 values being returned. Maybe you can also have a look into this?

There is a test described as should return the given array if the given number is greater than its length so it seems like this bug you mentioned is an intended behavior. I would rather change it in a different PR or even create an issue to change it.

The flattening looks good, thanks 👍

You're right about changing the behavior of pickRandom, we could consider it a breaking change in behavior. I've created a separate issue for it and we'll merge this into the first next breaking version v8.

Improvement is now available in v7.4.0 🎉

Hmm, so should I reopen this?

?

You didn't merge that

😱 I'm not sure what went wrong there, sorry! I really thought I had pressed the merge button.

Will fix this coverage today

I think the code coverage change is false alarm

Will merge now - for real 😉

Thanks :)

I'm glad you saw that I made a mistake here, thanks again!

Available now in v7.5.0

Hi, I accidentally commented on the wrong issue before. I just wanted to say that sometimes the behaviour of returning a vector instead of a value is what I prefer. For example, when I store a set of points in a list, each point represented as 2-dim vector, I expect pickRandom to return a random point, not the position of a random point along a random axis. Perhaps we could allow some property to distinguish n-dim arrays from lists of (n-1)-dim arrays, like NumPy or Julia already do it.

Thanks for your input @fweth , would be good to support both situations indeed. I think passing an option could be a solution here, in our case it's not possible I think to distinguish an n-dim array from a list with arrays, that will get confusing (mixing Arrays with Matrices etc).

See also #1990

Oh, I didn't realize that math.js already distinguishes between arrays and matrices. Wouldn't it make sense then that pickRandom applied to a matrix returns a random entry (not random vector) but applied to array returns random element (no matter what type)?

Hm, I think that would be confusing too. So far, the behavior of all functions is consistent independent of whether you're using an Array or a Matrix. Using either an Array or a Matrix is just a different "jacket" around the same data.

@fweth @josdejong but it's working like that now after I added matrix flattening

Yes I know, I hadn't expected this could impact how the function was used in practice.

New option is published now in v8.0.0 (see #1990 for details)