-
Notifications
You must be signed in to change notification settings - Fork 175
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
Implement sample_every argument for trange. #1384
Conversation
35733ea
to
b62a582
Compare
0164987
to
0984895
Compare
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.
So, the original intention of dt
was to do essentially what this PR does with the new sample_every
kwarg. It should not have been possible for trange
to return timesteps that were not actually executed by the simulator, so I would consider this PR an important bugfix rather than a new feature.
Because of that, I added some commits to essentially deprecate the dt
argument in favor of the sample_every
argument. Let me know if this makes sense @jgosmann and @Seanny123 and if so I'll rewrite the changelog message and merge.
Still LGTM. |
nengo/tests/test_simulator.py
Outdated
@@ -347,5 +345,5 @@ def test_sample_every_trange(Simulator, sample_every): | |||
with Simulator(model) as sim: | |||
sim.run(0.01) | |||
|
|||
assert_almost_equal( | |||
assert np.allclose( |
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.
Out of curiosity, why do you prefer np.allclose
over assert_almost_equal
?
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.
When an assertion is false, you get a traceback at that point. I'd rather that point be in the test itself than in a function called by the test, as I will often debug with pytest --pdb
and it's a slight annoyance to move up the call stack to find the test frame.
I don't know whether I missed the notification or I didn't get one for the approval comment, but the changes look good to me. |
This creates matching tranges for probes with a sample_every parameter that is not a multiple of the simulator timestep. Note that this essentially replaces the old dt argument. For backwards compatibility reasons, it still exists as a keyword argument, but will warn and use the logic of sample_every. Fixes #1368.
ae2d482
to
ddb4a19
Compare
Motivation and context:
This allows to create matching tranges for probes with a sample_every parameter that is not a multiple of the simulator timestep.
Fixes #1368.
Interactions with other PRs:
none
How has this been tested?
added a test
How long should this take to review?
Types of changes:
Checklist: