shuffling of lists and arrays

[UnixJunkie]

No description provided.

[UnixJunkie]

I know the new unit tests I added are not being run.
Any idea why?

[c-cube]

It's probably because qtest is not given the proper .ml{,i}v files. In any case I'm not sure how qtest would handle the conditional compilation parts :s

[gasche]

(I haven't looked at the qtest issue yet.)

Calling self_init() from a library function is wrong: you should let the user manage their random state, which is super-important if they want to be able to impose global determinism with a seed. Have you considered just reusing the implementation that is in BatRandom?

[UnixJunkie]

BatArray.shuffle comes from BatEnum indeed.
I can modify the None case so that Random.self_init is not called.

[gasche]

I think the behavior I would expect as a user is:

• if I pass a random state, it is used
• if I don't pass a random state, the global state is used

Your call to set_state is also wrong: if I pass an explicit random state, it should behave as a "pure" function that only modifies the random state (your implementation modifies the global state). What you should do instead (if you want an API where one can explicitly pass a random state) is use Random.State.int all the time, and use Random.get_state in the None case to get the current global state.

Have you considered taking an optional parameter instead of an option argument? I think your choice is fine, I'm just noting that those are the two options (no pun intended).

[UnixJunkie]

Is it fine to start using Random without having ever called Random.self_init() ?

[c-cube]

imho it's almost never fine to use Random rather than Random.St anyway. But don't call self_init in library code, it will confuse people that already called it, or even people who set the initial state manually to reproduce some results.

[UnixJunkie]

I learned about Random.State today. Better late than never ...

[thizanne]

Regarding the two "options", I think I'd personnally rather have an optional parameter rather than an option. It isn't really heavier to write ~rnd_st:state rather than (Some state) when you actually need it, and when you don't, it feels a little bit clusmy to be forced to pass a None parameter everywhere (and I feel like relying on a global state is the normal use-case of a shuffling function).

Why not both though ? Random has different functions (in Random.State) if you want an explicit state, it doesn't feel wrong that functions based on random behaviour also offer the two interfaces.

[UnixJunkie]

I went for the optional parameter since I also thought about it in the past.
I still cannot run the unit tests I added.
Also, I cannot get rid of introducing a src/shuffle.ml file.

[UnixJunkie]

I have updated the code in my branch.
What are the opinions about the current code?

[UnixJunkie]

There are still several problems: the tests I added are not run (you can check by falsifying anyone of them).
I'd like a way to remove the src/shuffle.ml file if possible.

[gasche]

I don't have time to look at it this week, I hope to look at the test issue this week-end or next week.

[gasche]

I fixed the unit-tests issue in the now-merged #715 (thanks for catching the issue!). Could you now check that your unit-tests are as expected?

I looked at your "need an extra shuffle.ml file" question, but I am not sure this is a problem. The reason why you have a third file, if I understand correctly, is to be able to share the implementation between BatArray and BatList without introducing a dependency between both modules. That seems to be a good idea, so why would you want to not do this? Another option, of course, is to just duplicate the same implementation in both files (I think it would be acceptable here, given that it is very short and relatively simple).

If we decide to keep an internal file, it should have a more unique name than Shuffle. What about src/batInnerShuffle.ml for example?

[UnixJunkie]

@gascher yes I don't want to introduce inter module dependencies. And yes I will probably go with your suggested name.

[UnixJunkie]

superseded by #718

[UnixJunkie]

@gascher yes the tests were correctly repaired; that's cool. Thanks.