Promote processes as a core part of Nengo #955
Conversation
|
We might want to do this after #945, since that moves this stuff around a bit. |
| @@ -26,11 +26,13 @@ | |||
| from .learning_rules import PES, BCM, Oja, Voja | |||
| from .params import Default | |||
| from .probe import Probe | |||
| from .processes import (BrownNoise, FilteredNoise, PresentInput, Process, | |||
| WhiteNoise, WhiteSignal) | |||
There was a problem hiding this comment.
This is a general question: when do we want things to become a "core" part of nengo, by which I mean be accessible simply with nengo.<class> rather than by nengo.<module>.<class>? Currently we don't have any of the distributions as core things; I think Gaussian and Uniform are so common that they could be core. With respect to processes, I would have Process and WhiteSignal for sure, WhiteNoise and FilteredNoise very probably, PresentInput probably, and BrownNoise maybe.
One thing to remember is once we promote something, it's hard to demote it without breaking code, whereas promoting things later is always fine because it will never break anything.
There was a problem hiding this comment.
Not sure how much sense it makes to have only parts of a module as core part. If I have to think about whether I can use a specific class with nengo.Foo or it has to be nengo.dists.Foo/nengo.processes.Foo, I'd rather always type nengo.dists.Foo/nengo.processes.Foo.
There was a problem hiding this comment.
Not sure how much sense it makes to have only parts of a module as core part
We do currently have only parts in the core, either intentionally or unintentionally. For example nengo.synapses.Triangle. I guess since we're only going to be having a limited set of things in this repository anyway (e.g. new neuron types will mostly go in user repositories or some "extras" repo), that's another reason to import everything in a module. I'm thinking particularly with dists though, that there are some I'd like to be easily accessible, but I'm worried adding them all will clutter up the core namespace. It is a good point, though, that users don't want to worry about what may be imported and what's not. Furthermore, if something isn't imported users might not realize it exists if they don't know e.g. only some synapses are imported.
One argument for importing less is that it might get confusing what is the purpose of some of the base objects. For example, is nengo.Gaussian a distribution, process, or neuron type?
There was a problem hiding this comment.
Hmm... yeah, looking back at this, I'm leaning towards just keeping the processes module import and having people use the full name. The dists name is intentionally short so that it isn't too bad to do nengo.dists.whatever. But yeah, we should maybe be more consistent and import either none or all of a particular module, so I'll make sure we're doing that.
hunse
force-pushed
the
promote-process
branch
3 times, most recently
from
4e13de5
to
6a210a9
Compare
tbekolay
force-pushed
the
promote-process
branch
4 times, most recently
from
ce3ea2c
to
6c3d3a9
Compare
|
I've finished this PR now, so it's ready for review! With #959 merged, this is the last PR in the 2.1.0 release! Where has the time gone, am I right all? aaaaaanyway this doesn't change much code, mostly documentation stuff, but fortunately far less than #959 so it shouldn't be too bad to review. Tagging @drasmuss and @hunse for review, or anyone else that has time! Also @hunse I kept 7f31378 as a separate commit so you can see the changes I'm proposing to the process notebook. |
| Arguments to pass to `synapse.make_step`. | ||
| seed : int, optional | ||
| signal invariant to ``dt``. | ||
| synapse_kwargs : dict, optional (Default: None) |
There was a problem hiding this comment.
Maybe say the default is {}? Or do you think this is clearer?
There was a problem hiding this comment.
Not sure, this is what I've used more often in other docstrings... usually I say something like If None, ... but in this case it's pretty obvious that no arguments will be passed if you pass None, I think.
|
LGTM. Just that one minor comment. |
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "## `PresentInput`\n", |
There was a problem hiding this comment.
Unrelated, but this example just made me think that nengo.utils.functions.piecewise and PresentInput are doing very similar things. If we made PresentInput a little bit more general so that it could have different presentation times, we could merge them into one.
There was a problem hiding this comment.
It would be great to have a process for utils.functions.piecewise. I'd keep it separate from PresentInput, though, or at least have some version of it with fixed times (maybe if you pass a constant it could have fixed times, or a list of times it could have varying?). It's just easier to do in OCL if the times are fixed.
Also with piecewise it would be great to have linear interpolation, too.
Added a notebook giving a brief introduction to processes, including the Process interface, some predefined processes, and how to make custom processes.
There is no reason to constrain FilteredNoise to use LinearFilter synapses.
tbekolay
force-pushed
the
promote-process
branch
from
6c3d3a9
to
11ea266
Compare
|
Made issue #1036 for the piecewise process and fixed the 1000-step typo, but left the arbitrary LTI thing as is; can always modify it later when we see how people are using the docs. |
This is another WIP PR to register that these things should be part of 2.1
Still to do in this PR:
SimProcessis documented / added to API docs