Fix random seed handling in simulation #668
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes incorrect random seed handling in the simulate_scenarios function. The bug caused the provided random_seed parameter to be ignored, potentially leading to unpredictable behavior in benchmarking operations.
Key Changes:
- Fixed the seed calculation logic to use the provided
random_seedinstead of always defaulting to_DEFAULT_SEED - Updated test to use a non-standard seed value to better detect such issues
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| baybe/simulation/scenarios.py | Corrected the seed assignment logic to properly use the random_seed parameter when provided |
| tests/test_simulation.py | Changed test seed value to a non-common number to avoid masking similar bugs |
| CHANGELOG.md | Documented the bug fix |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ): | ||
| """Callable for xyzpy simulation.""" | ||
| data = None if initial_data is None else initial_data[Initial_Data] | ||
| seed = None if random_seed is None else Monte_Carlo_Run + _DEFAULT_SEED |
There was a problem hiding this comment.
@AdrianSosic but wasnt random seed always specified as part of benchmarks? So it should not happen that it is None?
There was a problem hiding this comment.
The thing is: according to the old logic, it becomes None when you explicitly specify it 😄 (i.e. the None right after the equation symbol is taken as the value)
There was a problem hiding this comment.
Ok maybe I am not getting something:
DS=1
for random_seed in [None,0]:
print(f'Out for random_seed {random_seed}:',None if random_seed is None else DS)
Out for random_seed None: None
Out for random_seed 0: 1
And dont we always specify random_seed in benchmarks?
There was a problem hiding this comment.
sorry, read my own code wrong 😅 the point is that the passed seed actually never entered the simulation. if you specify one, you get the default 1337 with increasing number, no matter what you specified. So the previous version is definitely wrong, and the new version makes my minimal example work – but I also haven't yet fully understood how it interacts with the active_dims. Regardless of that, the fix should be merged. Will try to drill further into "why" it fixes the active dims part
There was a problem hiding this comment.
it becomes None when you explicitly specify it is that correct? According to the deleted line it becomes none when you pass none -> correct (or at least not catastrophic). But if its NOT none, the random_seed would be ignored and always use the default seed (+ monte carlo run) -> that was the issue
(comment started before Adrians last comment was there)
3 participants
Fixes a wrong assignment for controlling the random seeds in
simulate_scenarios. Potentially, this was the cause for many weird effects noticed in the benchmarking module in the past.