-
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
Factored out sample
function
#1181
Conversation
How about |
Yeah, I can see that! What do you think about renaming the first argument to |
I'm fine with that; I'm also fine with |
Renaming and relocation both sound fine to me. |
LGTM 🍰 |
LGTM. |
7c4e7f0
to
16859d3
Compare
This is used in the builder and might be useful in other instances, so it's worth exposing higher up in the API.
16859d3
to
614e765
Compare
In order to check all the boxes above, I added a docstring, then realized we don't have the distributions anywhere in the docs. So I added them (and fixed up our current docstrings to print nicely) in ef5d221 and added the docstring and a changelog entry. I think those don't warrant two re-reviews, so @jgosmann can you take a look at this and merge if they look good to you? |
Motivation and context:
The ensemble tuning function in #871 needed access to the builder's
sample
function. We're moving #871 tonengo_extras
, but it's a good idea to move thesample
function anyhow, as it's used in several places.Originally @drasmuss moved this to
nengo.utils.builder
; I figured it's not bad to have innengo.dists
. Though, since it's not quite the same asDistribution.sample
we should perhaps rename it to something?sample_if_needed
or something?Interactions with other PRs:
Supercedes #871.
How has this been tested?
Ran the test suite locally. Doesn't require additional testing.
How long should this take to review?
Types of changes:
Checklist:
Still to do:
nengo.dists
, and whether we should keep it namedsample
or something more explicit.