prepare API to get/set RandomState instances unique to each batch index#4448
prepare API to get/set RandomState instances unique to each batch index#4448grafi-tt wants to merge 4 commits intochainer:masterfrom
Conversation
|
Sorry for not my responding so long. Thank you for making a smaller PR. I'll take a look. |
|
@delta2323 Many thanks, and congratulation for 4.0.0 RC release! |
|
|
||
| def dummy_state(): | ||
| raise NotImplementedError | ||
| with random_state.set_random_state(dummy_state): |
There was a problem hiding this comment.
It seems to me this context manager doesn't take any effect on the return batch. If so, are you thinking to modify the code in the PRs following this one?
|
|
||
|
|
||
| def get_random_state(): | ||
| """Get a :class:`~numpy.random.RandomState` during iteration. |
There was a problem hiding this comment.
I think "during iteration" is not needed as users can call this function any time, although the most expected use case is to call it during iterations.
There was a problem hiding this comment.
It would be better to note that returned instance is a thread-local variable.
| def get_random_state(): | ||
| """Get a :class:`~numpy.random.RandomState` during iteration. | ||
|
|
||
| The instance obtained is unique to each index on a batch. You can |
There was a problem hiding this comment.
From the same reason as the previous one, I would suggest to describe it as one of expected use cases.
|
|
||
| """ | ||
| state = _random_state.value | ||
| if not isinstance(state, numpy.random.RandomState): |
There was a problem hiding this comment.
What do you assume as a type of state other than RandomState?
| Returns: | ||
| A list of :class:`numpy.random.RandomState` instances. | ||
| """ | ||
| return [derive_random_state() for _ in range(num)] |
There was a problem hiding this comment.
I think six.moves.range is better because num can be arbitrarily large.
|
I added several comments. Could you check that? |
|
@grafi-tt Hi, can you spare time to work on this PR? But I heard from other members that we may not need this PR any more. How do you think? |
|
@delta2323 Sorry, I don't have enough time to work on the PRs. I'm fine with closing them. It seems Just out of curiously, what other members say? (If you can't reveal, it's OK.) |
|
Sorry for late response. Thank you for closing the PR. One of my member agreed with you in that it is enough to use |
Part of #4230.
iterators.get_random_state(to be called by user) anditerators.random_state.set_random_state(to be called by iterator implementation) are implemented, andget_random_stateis documented.Currently iterators set dummy state by
set_random_statewhich raisesNotImplementedErrorimmediately whenget_random_stateis called.