Updated Process interface #652
Conversation
|
Hmm, I'm not going to get a chance to look at this in detail until the weekend, but just to check, why did you end up going with the PythonClosure-state approach rather than the PythonClass-state approach? I'd thought that's where the consensus was after the #622 discussion. Also, why doesn't the One thing to keep in mind is that we don't have to merge Process and NodeOutput. If they do turn out to be close enough to be the same thing, then that can simplify things, but if there are particular reasons that the Process implementation you're proposing differs from the desired NodeOutput stuff, then we should keep them separate. I still think they're pretty close, but I'm not as familiar with the inner workings of the Process stuff. |
I think you're right, we were leaning towards the PythonClass-state approach. I had actually forgotten that when I did it, and the closure approach is also what the synapse classes use, so there's a nice parallel there.
Yes, I guess we should add that.
Very true. I did this PR based on what seemed to make sense for the existing Processes, without thinking much about how people might like to use them as node outputs. As it stands, what I have here should be quite powerful for doing various types of node outputs. But maybe we want a different interface; we could take the class approach for NodeOutputs, and the closure approach for Processes. |
hunse
force-pushed
the
processes
branch
5 times, most recently
from
2647bd9
to
43a03b8
Compare
|
I was wondering when this will be reviewed and whether the Process interface in nengo2 is going to be updated? I need this interface to translate the HBB tutorials from nengo1.4 to nengo2. |
This will get reviewed eventually, but things are still up in the air about the approach we will take. The PythonClosure approach is surprising to novice programmers, and the PythonClass approach is surprising to experienced programmers. I'm still hoping for a clear winner one way or the other. That said, it shouldn't be necessary for translating the HBB tutorials, as the WhiteNoise object still works as it is. Once this PR gets sorted out, we should be able to clean up the tutorial a bit, but it should work fine without it (unless I'm not understanding the problem). |
|
The problem is, @s72sue needs actual white noise for the tutorials, to inject to neuron currents. This is something that I added in this PR. In my opinion, we should go ahead and review and merge this, even if it's not the approach we ultimately want to take. I think it's considerably better than what we have now, and the user-facing side will look the same even if we later add a commit to change to the class approach (of course, if users have written their own processes, those would have to change, but we would have this resolved before 2.1 either way, so users would never have to see the intermediary step). That said, I'm happy to split out parts of this commit if we really want to wait to decide. I'm just getting a little frustrated with some of my PRs sitting around for a long time, and getting stale, and then having to be rebased again, as well as not being able to use the improvements in further PRs and projects, etc. |
|
Yes, if this is causing frustration with how long it's taking, then we should definitely move ahead and sort something out with it quickly. I'd taken the lack of discussion on this thread as an indication that it wasn't a high priority for anyone, and then lots of other things came up. Can we meet tomorrow (thursday) in person and talk it through? Basically, I want to get a clear understanding on what people making their own processes and what backend developers need to know about this system. I definitely agree that it's be good to get something merged in soon, but I also think we need to be clear on what this means for backends too (both in terms is supporting Processes as noise input to neurons and in terms of supporting custom Processes as part of Nodes, if we want to keep that part). |
|
I'm available tomorrow to talk about it. Whenever! |
|
@tcstewar and I were thinking 10:00 am.
|
|
Kay, sounds good. |
|
Okay, rebased and ready for review. |
This is more descriptive of its actual function, and lets us have a DistributionParam that just does distributions. Also moved these params to `dists.py`.
hunse
force-pushed
the
processes
branch
2 times, most recently
from
6deb2db
to
8c56f7d
Compare
|
Okay, made those changes! They took a little longer than I thought, but I figured out that WhiteSignal is actually naturally periodic (some weird Fourier stuff going on), so we don't need to do all that reflection magic, we can just loop through it periodically. This means that I'm less unhappy with the WhiteSignal now. I renamed EDIT: The changes were
|
|
Is white allowing different kinds of white (e.g. Gaussian/Uniform?) Eric Hunsberger wrote:
|
|
WhiteNoise allows for different distributions (Gaussian, Uniform, etc., even UniformHypersphere). WhiteSignal, though, is the one that we generate in the Fourier domain, and I've never quite figured out the relationship between different white noise distributions and how they appear in the Fourier domain. |
|
Okay, added another test to make sure things stay deterministic in Travis CI. I think this is ready to go, fingers crossed. |
|
Cool, will quickly re-review and presumably merge this evening or early tomorrow morning. |
|
Looking good to me. As a note to those following this discussion, the intent here is to get this merged in as it greatly improves the ability to control and generate noise in models. Right now, it also allows you to attach these noise Processes to a Node and use it for generating signals. The API for doing this is still a work in progress, and this PR is meant as a stepping stone in that direction. One thing to keep in mind is that we should NOT do an official release until we get the Node part of this sorted out. But this PR looks good to me as a step in the right direction. |
|
That reminds me actually, one thing that's missing is a changelog entry. It's a user-facing change so we should make one (or multiple in this PR's case) even if, before the release, we have the change or replace one of those entries. I'll add them in the merge if they don't magically appear before I review / merge 🌈 |
|
What's our general policy on the changelog? Is it the author's job or the merger's job to write it? I think we had said at one point it's the merger's job, but I think it makes more sense if the author does it, just so we have two pairs of eyes look at it just like everything else. My problem is I always forget. |
hunse
force-pushed
the
processes
branch
2 times, most recently
from
f583102
to
3484698
Compare
From what I learned from Wikipedia it's a special property of Gaussian white noise vectors that they stay Gaussian white noise vectors under orthogonal transformations. That would mean the generation in the Fourier transform is probably only possible for Gaussian white noise. |
But you can generate for example a uniform white noise signal and take the Fourier transform and get something, so it seems to me that there must be something in the frequency domain that unites all uniform white noise signals and makes them different from Gaussian white noise. That relationship could be quite complicated, though. Also, I'm not sure if this is at all useful to people practically, so I wouldn't worry too much about it. But it is an interesting question. |
Removed `build_pyfunc`, since it is easier just to make a SimPyFunc.
Also added LinearFilterParam, specific to LinearFilters, for use in future Processes.
|
Looks good. Having immutable objects would definitely be nice. |
Renamed StochasticProcess to Process (since it is now more general), and updated the interface as per discussion in #622. Other changes: - Renamed `WhiteNoise` to `WhiteSignal`, since (a) it is band-limited and thus "colored" not "white" noise, and (b) this name is a better reflection of how we use it, as a signal, not as noise.
It turns out the signal generated by WhiteSignal is actually periodic! This means we don't have to reflect it. I added a test to check that this periodicity works (i.e. it and its first derivative are continuous).
Specifically, we're worried about examples in .ipynb_checkpoints directories, but we might as well ignore all hidden directories.
This ensures that these tests are deterministic. Also, `utils/tests/test_neurons.py:test_rates_kernel` would occasionally fail in Travis CI (only occasionally because the simulator seed wasn't set!), so I relaxed the tolerance a bit.
One entry for the Process API changes, and one for Processes being reset by the Simulator.
|
Hey Eric, I was wondering how I can generate a uniform white noise i.e., chosen Can you please clarify? -Sue On Thu, May 14, 2015 at 5:19 PM, Eric Hunsberger notifications@github.com
|
|
No, you definitely want WhiteNoise. Something like |
|
hmm..okay, thanks. On Mon, May 18, 2015 at 3:10 PM, Eric Hunsberger notifications@github.com
|
As discussed in #622. See the first commit message for a summary of changes (there are a number).
The second commit fixes up the simulator reset so that it resets Processes.
For discussion, there are a number of name changes, so I want to make sure everyone is okay with those. Also, I wasn't sure whether we should try to "deprecate" the old
StochasticProcess.ffunction so that 2.0 code can still run. I don't think it's quite as easy as just having that function return a reference to the process itself, so I would probably not bother with it.