opened
prashanth018
username_0
<!--
If you are filing an issue that is not a bug report, please feel free to erase
this template and describe the issue as clearly as possible.
-->


### Summary of Problem
Have been going through random module and observed that the choice() method only supports 1-D array. I will update the feature specific details soon, but on a bigger picture the changes to _choiceUniform() look straight forward while changes to _choiceProbabilities() will need some tweaking. 


### Steps to Reproduce

**Source Code:**

```chapel
use Random;
const vec = 1..7 by 2;
const vec1 = 2..8 by 3;
var D = {vec,vec1};
var A:[D] int;
A = 3;

var r = new owned PCGRandomStream(int);
writeln(r.choice(A));
```

**Compile command:**
`chpl foo.chpl`

**Output:**
Throws an error: says choice supports only 1-D array.
Make Random.choice() method support multi-dimensional array
issue
created
prashanth018
username_0
@username_2 could you comment on this issue? I'll start working if its good to go. Thanks :)
comment
created
arjundashrath
username_1
@username_0 @username_2 can I work on it?
comment
created
prashanth018
username_0
@username_1 I'm working on this already :)
comment
created
lydia-duncan
username_2
@username_3 and @username_4 - I think of you two as the experts on the Random module.  Does it seem reasonable to propose that the `choice` method should support multi-dimensional arrays?
comment
created
bradcray
username_3
I've never really used `choice()`, so don't have any firsthand experience or motivators here, but I don't really see any reason not to support them (or even, for that matter, sparse or associative, though the implementation would be trickier and more expensive to maintain, so it may not be worth pursuing it until someone wants it).
comment
created
mppf
username_4
maybe @username_5  has an opinion here?
comment
created
ben-albrecht
username_5
Agreed. I'm not opposed to adding support for N-D arrays. There just hasn't been user requests for it yet.

One design question for N-D arrays to consider: Should it sample in 1 dimension or all dimensions? 

By sampling along 1 dimension, I mean sample along 1 axis and return an `(N-1)`-dimensional array, e.g. a 2D array samples along rows and returns a 1D array. This is the behavior of [`numpy.random.Generator`](https://numpy.org/doc/stable/reference/random/generated/numpy.random.Generator.choice.html#numpy.random.Generator.choice), which served as a reference for the 1D interface, but we can certainly diverge from that if we decide to sample all dimensions.
comment
created
lydia-duncan
username_2
This definitely seems like a good question to resolve before starting on the implementation :)
comment
created
prashanth018
username_0
Hi @username_5, The implementation I have in my mind is to sample it on N-dimensions. Lets say we have the below code.
```chapel
use Random;
const vec = 1..7 by 2; //size = 4
const vec1 = 2..8 by 3; //size = 3
const vec2 = 1..3; //size = 3
var D = {vec,vec1,vec2};
var C:[D] int;

forall (i,j,k) in D do
  C(i,j,k) = i*100 + j*10 + k;

var r = new owned PCGRandomStream(int);
writeln(r.choice(C, 4, false));
// prints 4 randomly chosen element across 3 dimensions
``` 
So this can be efficiently achieved by sampling a random number between 0 to C.size-1 and then using the [orderToIndex method](https://github.com/chapel-lang/chapel/blob/594f5de9e7f478b3e146ee776f2418b074eb6e2d/modules/internal/ChapelArray.chpl#L1898) of Chapel Array which returns the index given offset.
comment
created
ben-albrecht
username_5
Thinking more on this, I think both approaches could be useful to include in the Random module. I would be OK with extending `choice` to support multi-dimensional arrays in the way you suggest @username_0 (assuming no others involved here object). I think the `N-1` dimension sampling could be implemented in a different function in the future.
comment
created
prashanth018
username_0
I raised a [PR](https://github.com/chapel-lang/chapel/pull/17168) for this issue. It would be good if one of you shared your insights. Changes to choiceTestMultiDimensionalSupport.chpl file is not complete yet. I'm currently writing a utility file for tests. I'll be pushing these changes once the changes to random.chpl are confirmed. Thanks :)
comment
created
prashanth018
username_0
I'll start working on the `N-1` dimension sampling that @username_5 has suggested.
comment
created
prashanth018
username_0
@username_2 I'm planning to handle `N-1` sampling suggested by ben in a separate PR, if that is fine? Also, could you please review the changes? Thanks :)
comment
created
lydia-duncan
username_2
I was expecting someone more familiar with the code to review the change
comment
created
lydia-duncan
username_2
@username_5 - would you mind looking at it?
comment
created
ben-albrecht
username_5
I am a good person to look at it, but I won't get a chance to look until later this week (hopefully).
comment
created
prashanth018
username_0
Thanks @username_2 @username_5
comment
created
prashanth018
username_0
Hey @username_5, would you mind taking a look at the changes if you find time? Thanks :)
comment