Permute list in place #23
Conversation
There was a problem hiding this comment.
Thanks, looks good overall!
I think we want RNG as a class, but the permutation should just be a standalone function that makes an instance of RNG, does the permutation and discards of the RNG.
Another thing to figure out is how to make this function take 2 arguments: seed and epoch number.
Also, CI is failing on formatting, see https://github.com/delphi-suite/delphi?tab=readme-ov-file#formatting
src/delphi/train/permute.py
Outdated
| Generate a random number between 0 and 1 using minstd_rand from C++11 | ||
| """ | ||
| self.state = (self.state * 48271) % 4294967296 | ||
| return self.state / 4294967296 |
There was a problem hiding this comment.
let's just return the state as an int
can you add type hint for return btw?
There was a problem hiding this comment.
I don't understand why it should return the state, I refactored it in a different way
src/delphi/train/permute.py
Outdated
| Permute a list in place using Fisher-Yates shuffle | ||
| """ | ||
| for i in range(len(list)): | ||
| j = int(self.random_number_generator() * (i + 1)) |
There was a problem hiding this comment.
if random_number_generator() would return int then you just need to % i (or i+1?)
There was a problem hiding this comment.
I changed this to random uniform and created a generate_number, which generates a number from 0 to max (which was required for the correct shuffling algorithm)
|
I resolved most of your changes but there's some where I'm not sure what you meant, |
* Permute list in place * Actual tests * Move test to correct position and updated tests * Fixed bug, separated function * Adds epoch control * misc fixes * type hint * % m in shuffle_epoch --------- Co-authored-by: Jett <[email protected]>
I think this is good