Closed AleMuzzi closed 1 year ago
Hey @AleMuzzi , thanks again for this. I made a few edits, summarized as follows:
- i removed the optional seed argument from all the main functions, due to the risk of changing global state with calls to
np.random.seed(my_seed)
deep inside the package. This could mess with reproducibility when it comes to multi-threading / parallel processing for instance- instead, i just hung onto the seed arguments in a couple private (prefixed with
_
) methods ofsample_action
andsample_policy
that are used to check that the desired behaviour works in thetest_control.py
test suite. Additionally, when the seed is actually used in_select_highest_test(...)
, I switched to actually seeding a generator (usingnp.random.default_rng()
) so that we don't mess with the global state. This same approach could in theory be used for all the functions / methods of Agent (not just the unit-test-intended private ones), but that choice raises some questions in of itself, so for now I vote that we just keep the reproducibility-control in the unit-test-intended versions of those functions.Before I go ahead and merge, I'll wait to see if you have any thoughts/feedback/requests to revert back to your original version -- if not, I'll merge within a couple days if I don't hear from you.
Seems good to me 👍
Hey @AleMuzzi , thanks again for this. I made a few edits, summarized as follows:
np.random.seed(my_seed)
deep inside the package. This could mess with reproducibility when it comes to multi-threading / parallel processing for instance_
) methods ofsample_action
andsample_policy
that are used to check that the desired behaviour works in thetest_control.py
test suite. Additionally, when the seed is actually used in_select_highest_test(...)
, I switched to actually seeding a generator (usingnp.random.default_rng()
) so that we don't mess with the global state. This same approach could in theory be used for all the functions / methods of Agent (not just the unit-test-intended private ones), but that choice raises some questions in of itself, so for now I vote that we just keep the reproducibility-control in the unit-test-intended versions of those functions.Before I go ahead and merge, I'll wait to see if you have any thoughts/feedback/requests to revert back to your original version -- if not, I'll merge within a couple days if I don't hear from you.