-
Notifications
You must be signed in to change notification settings - Fork 174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set rng explicitly when caching solver. #1359
Conversation
nengo/cache.py
Outdated
if E is None and 'E' in args: | ||
E = defaults[args.index('E')] | ||
if rng is None: | ||
rng = np.random |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I ask if there's a reason to specify the default this way rather than in the arg list? I know putting mutable things as a default can be problematic, but in this case np.random
is mutable but also global so it would be the same were it mutated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am just following what's done in solvers.py
, but I was wondering the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I say change it both places 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did I change this... that's a good question! It might have been so that all the solvers would have the same default rng=None
, whether they actually use an rng
or not, and maybe I thought it kept the function signature a bit cleaner, too. But I'm fine making the change suggested here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in both places. :)
The np.random object is mutable, but also global. Thus, the idiom with a default of None is not required. See #1359 (comment) for further discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for changing all the solvers! 🌈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting the rng
in the argument freaked me out a little, because of this Python gotchya for mutable arguments, but I now understand that np.random
is not mutable in any significant way so it's not a problem.
The np.random object is mutable, but also global. Thus, the idiom with a default of None is not required. See #1359 for further discussion.
6ff3aa6
to
87faf37
Compare
Motivation and context:
As noted in #1358 trying to inspect the default arguments has two problems:
(*args, **kwargs)
idiom to write a wrapper for solvers.None
and retrievenp.random
later. So the RNG state was not saved anyways when relying on the default arguments (but I believe the builder always sets therng
argument, so that this bug did not ocurr in practice).Instead of inspecting the defaults,
E
is assumed to be alwaysNone
by default (any other default would seem quite weird to me) andrng
is set tonp.random
ifNone
.By setting the RNG explicitly, all cached solvers will depend on the RNG state. This means that decoders are only reused if the RNG state matches even though the solver might be independent of the RNG. Unfortunately, there is no way to know whether the solver will use the RNG and this issue did exist before.
Fixes #1358.
Interactions with other PRs:
none
How has this been tested?
Unit tests still pass. I did not test the original issue reported in #1358, but as the failing code was completely removed, it should be resolved. Not sure if we should add a regression test. It seems weird to write a test that tests that the code isn't doing certain things and it seems unlikely that #1358 is reintroduced. I also did not add a test for the hashing of the RNG state because it seems to be quite awkward to test (without gaining much). But I could probably be swayed to add tests.
How long should this take to review?
Types of changes:
Checklist: