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

Setting delay before connect is confusing to users #737

Closed
thesamovar opened this Issue Aug 15, 2016 · 18 comments

Comments

Projects
None yet
3 participants
@thesamovar
Member

thesamovar commented Aug 15, 2016

If you set delay before you call connect, it doesn't have the intended effect for some users. I think maybe the best thing to do is to make this clearer in the docs (repeating ourselves if necessary) and raise a warning if you set delay on 0 synapses?

@thesamovar thesamovar added this to the 2.0 milestone Aug 15, 2016

@rat-h

This comment has been minimized.

Show comment
Hide comment
@rat-h

rat-h Aug 15, 2016

I believe it should be an error message and the exit from the script totally. Otherwise user will still run a simulation and will get a result. It is quite easy to miss or ignore a warning message in output.

rat-h commented Aug 15, 2016

I believe it should be an error message and the exit from the script totally. Otherwise user will still run a simulation and will get a result. It is quite easy to miss or ignore a warning message in output.

@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg Aug 16, 2016

Member

I believe it should be an error message and the exit from the script totally. Otherwise user will still run a simulation and will get a result. It is quite easy to miss or ignore a warning message in output.

It is often a difficult choice between a warning and an error and I agree that it is easy to overlook a warning. However, we also should not take raising an error lightly, in particular for code that is correct albeit probably a mistake (as in the case of setting a synaptic variable on 0 synapses). One use case I am seeing where an error would be really annoying is a network with probabilistic synaptic connections. Especially if you have many different types of connections in a sparsely connected network it is possible that you might have a run where 0 synapses are created for a connection type. This will probably not happen during a normal run (and you'll get a warning about simulating a Synapses without any actual synapses for a good reason) but it might come up for example when you quickly scale down the size of your network for testing. In that case, wrapping every synapses.delay = ... in a if len(synapses) > 0: just to avoid this error would be very annoying. Even more problematic, it will not work in standalone mode as we don't know the number of synapses before the code is actually executed. I therefore think that a warning is a better choice here.

A final remark: if you set the delay to a constant value for all synapses (as was the case in the original report on the mailing list), then there is a preferable syntax which cannot go wrong no matter how many synapses: you can set the delay as an argument of the Synapses call itself:

synapses = Synapses(source, target, model='...', on_pre='...', delay=const_delay)
Member

mstimberg commented Aug 16, 2016

I believe it should be an error message and the exit from the script totally. Otherwise user will still run a simulation and will get a result. It is quite easy to miss or ignore a warning message in output.

It is often a difficult choice between a warning and an error and I agree that it is easy to overlook a warning. However, we also should not take raising an error lightly, in particular for code that is correct albeit probably a mistake (as in the case of setting a synaptic variable on 0 synapses). One use case I am seeing where an error would be really annoying is a network with probabilistic synaptic connections. Especially if you have many different types of connections in a sparsely connected network it is possible that you might have a run where 0 synapses are created for a connection type. This will probably not happen during a normal run (and you'll get a warning about simulating a Synapses without any actual synapses for a good reason) but it might come up for example when you quickly scale down the size of your network for testing. In that case, wrapping every synapses.delay = ... in a if len(synapses) > 0: just to avoid this error would be very annoying. Even more problematic, it will not work in standalone mode as we don't know the number of synapses before the code is actually executed. I therefore think that a warning is a better choice here.

A final remark: if you set the delay to a constant value for all synapses (as was the case in the original report on the mailing list), then there is a preferable syntax which cannot go wrong no matter how many synapses: you can set the delay as an argument of the Synapses call itself:

synapses = Synapses(source, target, model='...', on_pre='...', delay=const_delay)
@thesamovar

This comment has been minimized.

Show comment
Hide comment
@thesamovar

thesamovar Aug 16, 2016

Member

I agree it shouldn't be an error.

A related point, perhaps we ought to allow a syntax like this?

S = Synapses(G, G, 'w:1', on_pre='v_post += w')
S.connect(p=0.1, delay=3*ms, w=0.1)
S.connect(p=0.2, delay=5*ms, w=0.2)

I'm not actually sure there is a way to do this at the moment, and it makes it clear that the delay applies just to that connect call, etc. What do you think?

Member

thesamovar commented Aug 16, 2016

I agree it shouldn't be an error.

A related point, perhaps we ought to allow a syntax like this?

S = Synapses(G, G, 'w:1', on_pre='v_post += w')
S.connect(p=0.1, delay=3*ms, w=0.1)
S.connect(p=0.2, delay=5*ms, w=0.2)

I'm not actually sure there is a way to do this at the moment, and it makes it clear that the delay applies just to that connect call, etc. What do you think?

@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg Aug 16, 2016

Member

Hmm, interesting idea. Let's open a new issue for this and discuss it post-2.0. Setting synaptic variables for specific connect calls is also related to #263.

Member

mstimberg commented Aug 16, 2016

Hmm, interesting idea. Let's open a new issue for this and discuss it post-2.0. Setting synaptic variables for specific connect calls is also related to #263.

@thesamovar

This comment has been minimized.

Show comment
Hide comment
@thesamovar

thesamovar Aug 16, 2016

Member

Agreed it has to be post-2.0. Will open an issue. So for this issue are we agreed that it's worth adding a warning and a couple of sentences in the docs, closing this pre-2.0?

Member

thesamovar commented Aug 16, 2016

Agreed it has to be post-2.0. Will open an issue. So for this issue are we agreed that it's worth adding a warning and a couple of sentences in the docs, closing this pre-2.0?

@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg Aug 16, 2016

Member

So for this issue are we agreed that it's worth adding a warning and a couple of sentences in the docs, closing this pre-2.0?

Yep.

Member

mstimberg commented Aug 16, 2016

So for this issue are we agreed that it's worth adding a warning and a couple of sentences in the docs, closing this pre-2.0?

Yep.

@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg Aug 16, 2016

Member

One more thought on this, potentially addressing both @rat-h's and our concerns: there are two different situations, one is setting a synaptic variable without any preceding connect call, the other is setting a synaptic variable after a connect that resulted in the generation of 0 synapses. My arguments against raising an error (probabilistic connections, standalone mode) do apply to the latter case but not the former.
Raise an error if there's no connect call but only a warning if there was a connect call but it resulted in 0 synapses? For the latter case, there's also the question when to raise the warning: at the time of the connect, for each assignment to a synaptic variable that applies to "0 synapses"?

Member

mstimberg commented Aug 16, 2016

One more thought on this, potentially addressing both @rat-h's and our concerns: there are two different situations, one is setting a synaptic variable without any preceding connect call, the other is setting a synaptic variable after a connect that resulted in the generation of 0 synapses. My arguments against raising an error (probabilistic connections, standalone mode) do apply to the latter case but not the former.
Raise an error if there's no connect call but only a warning if there was a connect call but it resulted in 0 synapses? For the latter case, there's also the question when to raise the warning: at the time of the connect, for each assignment to a synaptic variable that applies to "0 synapses"?

@thesamovar

This comment has been minimized.

Show comment
Hide comment
@thesamovar

thesamovar Aug 16, 2016

Member

That sounds like a great solution to me.

I would raise a warning for each assignment to a synaptic variable that applies to 0 synapses, or perhaps just one warning for the first time the user does it?

Member

thesamovar commented Aug 16, 2016

That sounds like a great solution to me.

I would raise a warning for each assignment to a synaptic variable that applies to 0 synapses, or perhaps just one warning for the first time the user does it?

@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg Aug 16, 2016

Member

I would raise a warning for each assignment to a synaptic variable that applies to 0 synapses, or perhaps just one warning for the first time the user does it?

I'd say once for each variable, but I don't have a strong opinion. I somewhat feel that just having a connect that results in 0 new synapses might merit a warning, too, but on the other hand you'll already get a warning at the beginning of the run if a Synapses object does not have any synapses -- this would then be redundant in the common use case of a single connect call.

Member

mstimberg commented Aug 16, 2016

I would raise a warning for each assignment to a synaptic variable that applies to 0 synapses, or perhaps just one warning for the first time the user does it?

I'd say once for each variable, but I don't have a strong opinion. I somewhat feel that just having a connect that results in 0 new synapses might merit a warning, too, but on the other hand you'll already get a warning at the beginning of the run if a Synapses object does not have any synapses -- this would then be redundant in the common use case of a single connect call.

@rat-h

This comment has been minimized.

Show comment
Hide comment
@rat-h

rat-h Aug 16, 2016

Just add to discussion. The first case (when user try to set delay before call connect function) may works similar to
synapses = Synapses(source, target, model='...', on_pre='...', delay=const_delay)
So you don't need to raise any warring or error; it is just default setting for synapses. What do you think?

rat-h commented Aug 16, 2016

Just add to discussion. The first case (when user try to set delay before call connect function) may works similar to
synapses = Synapses(source, target, model='...', on_pre='...', delay=const_delay)
So you don't need to raise any warring or error; it is just default setting for synapses. What do you think?

@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg Aug 16, 2016

Member

Thanks for the comment. I see what you mean, but I think it would be somewhat inconsistent if setting a delay before a connect has a different semantics. Also, this does not solve the issue for other synaptic variables such as weights, there is no equivalent mechanism in that case.
Finally, there are some issues to this from the implementation side. Setting the delay uses the very general mechanism to set a state variable, at that level there is nothing special to delays and we'd need to special-case it just to support this new feature. In addition, we allocate memory in the beginning, either for a single delay (if specified as part of the Synapses initializer`), or a dynamic array for one delay per synapse that grows with the number of synapses.

Member

mstimberg commented Aug 16, 2016

Thanks for the comment. I see what you mean, but I think it would be somewhat inconsistent if setting a delay before a connect has a different semantics. Also, this does not solve the issue for other synaptic variables such as weights, there is no equivalent mechanism in that case.
Finally, there are some issues to this from the implementation side. Setting the delay uses the very general mechanism to set a state variable, at that level there is nothing special to delays and we'd need to special-case it just to support this new feature. In addition, we allocate memory in the beginning, either for a single delay (if specified as part of the Synapses initializer`), or a dynamic array for one delay per synapse that grows with the number of synapses.

@thesamovar

This comment has been minimized.

Show comment
Hide comment
@thesamovar

thesamovar Aug 16, 2016

Member

Generally I prefer fewer warning messages because it's less annoying and makes users more likely to read them (if there are loads of messages you're more likely to just ignore them I feel), and just one is enough to highlight that there is a problem that needs to be fixed. So I think my ideal on reflection would be a single warning if the user creates 0 synapses, and no additional warnings for each variable. The case of setting a variable before calling connect would be handled by the error.

@rat-h I'd say that solution makes sense, but one of our design principles in Brian 2 is not to have the same syntax mean different things in different scenarios which your proposal would violate. I think in the long term this helps make it clear what Brian is doing.

Member

thesamovar commented Aug 16, 2016

Generally I prefer fewer warning messages because it's less annoying and makes users more likely to read them (if there are loads of messages you're more likely to just ignore them I feel), and just one is enough to highlight that there is a problem that needs to be fixed. So I think my ideal on reflection would be a single warning if the user creates 0 synapses, and no additional warnings for each variable. The case of setting a variable before calling connect would be handled by the error.

@rat-h I'd say that solution makes sense, but one of our design principles in Brian 2 is not to have the same syntax mean different things in different scenarios which your proposal would violate. I think in the long term this helps make it clear what Brian is doing.

@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg Aug 16, 2016

Member

So I think my ideal on reflection would be a single warning if the user creates 0 synapses, and no additional warnings for each variable. The case of setting a variable before calling connect would be handled by the error.

Ok, I think I am fine with this. This would also mean removing the warning about 0 synapses at simulation start to only have a single warning, correct?

I just realized that raising the error for a missing connect is a bit trickier than I thought. Setting the synaptic variables goes through our standard state variable machinery, there is nothing synapses-specific where we could check for a connect_was_called flag. Of course we could do something along the lines of if getattr(owner, 'connect_was_called', True), but this is kind of ugly. I see whether I can find a more elegant solution tomorrow.

Member

mstimberg commented Aug 16, 2016

So I think my ideal on reflection would be a single warning if the user creates 0 synapses, and no additional warnings for each variable. The case of setting a variable before calling connect would be handled by the error.

Ok, I think I am fine with this. This would also mean removing the warning about 0 synapses at simulation start to only have a single warning, correct?

I just realized that raising the error for a missing connect is a bit trickier than I thought. Setting the synaptic variables goes through our standard state variable machinery, there is nothing synapses-specific where we could check for a connect_was_called flag. Of course we could do something along the lines of if getattr(owner, 'connect_was_called', True), but this is kind of ugly. I see whether I can find a more elegant solution tomorrow.

@thesamovar

This comment has been minimized.

Show comment
Hide comment
@thesamovar

thesamovar Aug 16, 2016

Member

Well we'd still want to warn if the user never called connect() at all. But if we make it the same warning message and set it to warn only once, it should handle that I think?

Member

thesamovar commented Aug 16, 2016

Well we'd still want to warn if the user never called connect() at all. But if we make it the same warning message and set it to warn only once, it should handle that I think?

@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg Aug 16, 2016

Member

Well we'd still want to warn if the user never called connect() at all. But if we make it the same warning message and set it to warn only once, it should handle that I think?

Ah, right. I don't think the same warning for the two cases is a good idea, but we can easily check for the "no connect" case with the same flag we use for the error message. Actually, wouldn't it be more consistent to raise an error in this case, too? You can always set the active flag to False if for whatever reason you need to have this Synapses object without synapses around...

Member

mstimberg commented Aug 16, 2016

Well we'd still want to warn if the user never called connect() at all. But if we make it the same warning message and set it to warn only once, it should handle that I think?

Ah, right. I don't think the same warning for the two cases is a good idea, but we can easily check for the "no connect" case with the same flag we use for the error message. Actually, wouldn't it be more consistent to raise an error in this case, too? You can always set the active flag to False if for whatever reason you need to have this Synapses object without synapses around...

@thesamovar

This comment has been minimized.

Show comment
Hide comment
@thesamovar

thesamovar Aug 16, 2016

Member

Yes, I'd be happy for it to be an error not to create any synapses.

Member

thesamovar commented Aug 16, 2016

Yes, I'd be happy for it to be an error not to create any synapses.

@rat-h

This comment has been minimized.

Show comment
Hide comment
@rat-h

rat-h Aug 17, 2016

BRIAN's fathers. It seems I've found a solution. We mix up two different parameter sets. First is parameters by default and second is parameters for existing connections. All problem on this level.

I don't know Brian inner machinery, so I can suggest only user level syntax. I'm trying to help you to remove ambiguous part of the user interface. Below is how I see the solution.

For parameters by default:
s = Synapses(source, target, model='...', on_pre='...', parameter=value .....)
example:
s = Synapses(source, target, model=' w : 1', on_pre='v_post +=w ', w=0.5, delay=2.1)

User can change default parameters later:
s.setDefault( delay=3.1)
or more pythonial style:
s.default.delay=3.1
s.default.w=0.8

Now if user creates connections, it is possible to access to each parameter of these connections
s.connect(p=0.1)
s.delay=3.4

Finally code below should trigger an error
s = Synapses(source, target, model=' w : 1', on_pre='v_post +=w ', w=0.5, delay=2.1)
s.delay = 0.5
because it try to set up delay for connections which don't exist.

However code below should trigger just warning:
s = Synapses(source, target, model=' w : 1', on_pre='v_post +=w ', w=0.5, delay=2.1)
s.connect(p=0)
s.delay = 0.5
and indicates that parameter was applied for zero connections.

Again it's just a suggestion. I may be totally wrong.

rat-h commented Aug 17, 2016

BRIAN's fathers. It seems I've found a solution. We mix up two different parameter sets. First is parameters by default and second is parameters for existing connections. All problem on this level.

I don't know Brian inner machinery, so I can suggest only user level syntax. I'm trying to help you to remove ambiguous part of the user interface. Below is how I see the solution.

For parameters by default:
s = Synapses(source, target, model='...', on_pre='...', parameter=value .....)
example:
s = Synapses(source, target, model=' w : 1', on_pre='v_post +=w ', w=0.5, delay=2.1)

User can change default parameters later:
s.setDefault( delay=3.1)
or more pythonial style:
s.default.delay=3.1
s.default.w=0.8

Now if user creates connections, it is possible to access to each parameter of these connections
s.connect(p=0.1)
s.delay=3.4

Finally code below should trigger an error
s = Synapses(source, target, model=' w : 1', on_pre='v_post +=w ', w=0.5, delay=2.1)
s.delay = 0.5
because it try to set up delay for connections which don't exist.

However code below should trigger just warning:
s = Synapses(source, target, model=' w : 1', on_pre='v_post +=w ', w=0.5, delay=2.1)
s.connect(p=0)
s.delay = 0.5
and indicates that parameter was applied for zero connections.

Again it's just a suggestion. I may be totally wrong.

@mstimberg

This comment has been minimized.

Show comment
Hide comment
@mstimberg

mstimberg Aug 17, 2016

Member

Thanks for your suggestions. I think I addressed the most important part (raising an error if setting a variable without a preceding connect) in PR #739 . The rest are fundamental changes/additions to the syntax and we will therefore not tackle them before the release of 2.0 (which we will do in September). We should continue to discuss this in a separate issue -- I am not yet sure what the best syntax would be (e.g. it might be better to have default/initial values directly as part of the equations instead of as keywords in the initializer call).

Member

mstimberg commented Aug 17, 2016

Thanks for your suggestions. I think I addressed the most important part (raising an error if setting a variable without a preceding connect) in PR #739 . The rest are fundamental changes/additions to the syntax and we will therefore not tackle them before the release of 2.0 (which we will do in September). We should continue to discuss this in a separate issue -- I am not yet sure what the best syntax would be (e.g. it might be better to have default/initial values directly as part of the equations instead of as keywords in the initializer call).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment