-
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
Added min_voltage parameter to nengo.LIF #666
Conversation
This adds to the "Nengo should have a spec document for backends" call. Noted that I need to implement this. |
Yup, we're definitely needing that sort of document. I'm in the midst of doing that to a certain degree with this https://github.com/ctn-waterloo/nengo_reference backend by identifying a set of nengo tests that all backends should pass. But there's also going to be optional features that a backend may or may not support (in which case there should also be ways of the backend indicating what it supports and doesn't support). And it means we have to be much more careful about adding features that affect backends. For this particular feature, I'd say it's completely optional for a backend to support it. :) |
@@ -225,12 +225,16 @@ class LIF(LIFRate): | |||
|
|||
probeable = ['spikes', 'voltage', 'refractory_time'] | |||
|
|||
def __init__(self, min_voltage=0, **kwargs): |
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.
This changes the order of parameters for LIF. Are we sure that no-one is doing LIF(0.03, 0.003)
without the keywords? If they are, is it their own damn fault if this breaks their code?
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.
We shouldn't change the order of parameters. And in general, I don't think we should use **kwargs
if we can avoid it (and we can in this case)
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.
Agreed. I'll make a fix.
Okay, changed the parameter order. LGTM now. |
Cool, I'll take a quick look and merge. I will probably add some labels to the plots in |
I just added a maximum to |
I think that makes sense assuming that the reset voltage is fixed. We could make that configurable too, though I'm not sure of any reason why anyone would particularly want to do that. |
513cebe
to
3316492
Compare
I approve. |
Looks great to me. Thanks for polishing this up! :) |
72a8b8a
to
5e8bc88
Compare
This is needed for one version of the adaptive LIF neuron. As mentioned #644 (comment) it was felt that having a parameter like this might also be useful for hyperpolarizing neurons too.