-
Notifications
You must be signed in to change notification settings - Fork 4
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
Electrode current in lfp #52
Conversation
…to allow electrode to contirbute to LFP signal
@@ -227,6 +227,7 @@ A continuous injection of current. | |||
============================== ========== ============ ========================================== | |||
amp_start float Mandatory The amount of current initially injected when the stimulus activates. Given in nA. | |||
amp_end float Optional If given, current is interpolated such that current reaches this value when the stimulus concludes. Otherwise, current stays at amp_start. Given in nA. | |||
represents_physical_electrode boolean Optional Default is False. If True, the signal will be implemented using a NEURON IClamp mechanism. The IClamp produce an electrode current which is not included in the calculation of extracellular signals, so this option should be used to represent a physical electrode. If the noise signal represents synaptic input, `represents_physical_electrode` should be set to False, in which case the signal will be implemented using a MembraneCurrentSource mechanism, which is identical to IClamp, but produce a membrane current, which is included in the calculation of the extracellular signal. |
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.
If I'm understanding this right, the default value won't be a vanilla IClamp anymore? (what did it used to be in neurodamus @jorblancoa / @WeinaJi / @ferdonline ?). Instead, MembraneCurrentSource
is substituted, which uses NONSPECIFIC_CURRENT i
instead of ELECTRODE_CURRENT i
?
Will this impact how simulations are run, since the default is different?
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.
Yes, that is correct (the default was previously a vanilla IClamp). This will not have any impact on how simulations are run. It will have an impact on reporting, since now the stimulus current will be included in i_membrane
instead of IClamp
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.
It will have an impact on reporting, since now the stimulus current will be included in i_membrane instead of IClamp
I think we'll need to have broad confirmation that everyone who relies on reporting know about this then, since it's a change that it impacts them. Probably @MWolfR, @romani79 should get their team to comment, @darshanmandge / @anilbey probably need to know, and likely others.
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 just discussed it with @joseph-tharayil. This change does not introduce any issues on bluecellulab side since the voltage traces produces will be identical.
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.
The changes does not affect any other single cell software. So, it is good from my side.
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.
Can we move forward with this and the PR in libsonata?
BlueBrain/libsonata#354
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.
It's been communicated to the circuits and connectomics teams, and no one's raised any objections yet, so I think we can move forward
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.
It's been communicated to the circuits and connectomics teams,
Great! Can we get an ok on this ticket from them?
I just want to confirm that everyone is aware that we're moving away from a default that has been the standard for many years, to something different.
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.
Looks good to me.
The change to default behavior is only affecting reports for niche use cases and in a transparent manner and in can be overridden. Approve.
Me and the circuit team have not had the opportunity to look into the details of the proposed changes. However, I appreciate that the old setup can be still obtained with represents_physical_electrode:true. If this is the case, I am not contrary to the changes, so you have my green light on that. |
thanks for getting everyone in the loop, @joseph-tharayil and @jorblancoa; let's do this |
I've added a
represents_physical_electrode
key, which defaults to False, for all of the noise input types. If the key is set to False, then the stimulus should be implemented as either a conductanceSource or a MembraneCurrentSource, both of which are implemented here. In these cases, the current will be a membrane current, which will contribute to the extracellular signalIf the key is set to True, then the stimulus should be implemented as an SEClamp (if a conductance source) or an IClamp (if a current source). The SEClamp and IClamp should not contribute to the extracellular current. This way, the user can specify whether each source individually affects the extracellular signal, rather than having to make an all-or-nothing decision on the IClamp currents