Skip to content
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

[BBPBGLIB-1137] Account for nonzero total current when using SEClamp; allow nonzero total current when simulating physical electrodes #134

Merged
merged 68 commits into from
May 10, 2024

Conversation

joseph-tharayil
Copy link
Member

@joseph-tharayil joseph-tharayil commented Feb 27, 2024

Context

NEURON reports SEClamp currents at different time points than all other currents, thus giving inaccurate results in lfp calculations, in cases where the SEClamp represents synaptic inputs.

This can be fixed by replacing the SEClamp with a new point process mechanism that is identical, except for producing a NONSPECIFIC_CURRENT instead of an ELECTRODE_CURRENT.

However, in cases where SEClamp or IClamp are used to represent physical electrodes, rather than synaptic inputs, the currents should not be included in the LFP calcualtion

Scope

When a current_source or conductance_source is used, we check if the alternative mod file, which is expected to be named 'MembraneCurrentSource.mod' or 'ConductanceSource.mod' is available. If so, it is used, with nothing else changed, unless the key-value pair "represents_physical_electrode" is set to True in the Stimulus Config file. Otherwise the IClamp or SEClamp is used as before.

Testing

Review

  • PR description is complete
  • Coding style (imports, function length, New functions, classes or files) are good
  • Unit/Scientific test added
  • Updated Readme, in-code, developer documentation

@joseph-tharayil joseph-tharayil changed the title New conductance source [BBPBGLIB-1137] Account for nonzero total current when using SEClamp Feb 27, 2024
@bbpbuildbot

This comment has been minimized.

@jorblancoa jorblancoa marked this pull request as ready for review February 28, 2024 10:19
@bbpbuildbot

This comment has been minimized.

neurodamus/core/stimuli.py Outdated Show resolved Hide resolved
neurodamus/stimulus_manager.py Outdated Show resolved Hide resolved
@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@joseph-tharayil
Copy link
Member Author

One suggestion we recently got was to add an explicit test that, for a small network, the behavior is identical using the old and the new mod files. I'm not really sure how to implement this. Does anyone have any suggestions?

Maybe once the new parameter is deployed in libsonata, a scientific test could be added to run a small simulation twice with 'represents_physical_electrode' = true and false in order to check the report (and see that the first with i_membrane+IClamp and the second with only i_membrane gives the same results)

I've created the tests, and they run successfully with pytest, but when htey are run through the CI pipeline they cause a segfault. Any idea why this is?

@jorblancoa
Copy link
Collaborator

One suggestion we recently got was to add an explicit test that, for a small network, the behavior is identical using the old and the new mod files. I'm not really sure how to implement this. Does anyone have any suggestions?

Maybe once the new parameter is deployed in libsonata, a scientific test could be added to run a small simulation twice with 'represents_physical_electrode' = true and false in order to check the report (and see that the first with i_membrane+IClamp and the second with only i_membrane gives the same results)

I've created the tests, and they run successfully with pytest, but when htey are run through the CI pipeline they cause a segfault. Any idea why this is?

To be honest i didnt check in detail, but in any case it wont work properly until the change in represents_physical_electrode is added to libsonata and there is a new release, so i would wait for that before investigating more.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@jorblancoa jorblancoa requested a review from WeinaJi May 8, 2024 13:27
@jorblancoa jorblancoa closed this May 8, 2024
@jorblancoa jorblancoa reopened this May 8, 2024
@bbpbuildbot

This comment has been minimized.

docs/online-lfp.rst Outdated Show resolved Hide resolved
@bbpbuildbot
Copy link

@joseph-tharayil joseph-tharayil merged commit 1d2bf99 into main May 10, 2024
6 checks passed
@joseph-tharayil joseph-tharayil deleted the new_conductance_source branch May 10, 2024 07:33
atemerev pushed a commit that referenced this pull request May 24, 2024
…; allow nonzero total current when simulating physical electrodes (#134)

## Context
NEURON reports SEClamp currents at different time points than all other
currents, thus giving inaccurate results in lfp calculations, in cases
where the SEClamp represents synaptic inputs.

This can be fixed by replacing the SEClamp with a new point process
mechanism that is identical, except for producing a NONSPECIFIC_CURRENT
instead of an ELECTRODE_CURRENT.

However, in cases where SEClamp or IClamp are used to represent physical
electrodes, rather than synaptic inputs, the currents should not be
included in the LFP calcualtion

## Scope
When a current_source or conductance_source is used, we check if the
alternative mod file, which is expected to be named
'MembraneCurrentSource.mod' or 'ConductanceSource.mod' is available. If
so, it is used, with nothing else changed, unless the key-value pair
"represents_physical_electrode" is set to True in the Stimulus Config
file. Otherwise the IClamp or SEClamp is used as before.

## Testing
Scientific test added to ensure that new mechanisms produce identical membrane voltages to the old ones

## Review
* [x] PR description is complete
* [x] Coding style (imports, function length, New functions, classes or
files) are good
* [x] Unit/Scientific test added
* [x] Updated Readme, in-code, developer documentation

---------

Co-authored-by: Jorge Blanco Alonso <jorge.blancoalonso@epfl.ch>
Co-authored-by: Jorge Blanco Alonso <41900536+jorblancoa@users.noreply.github.com>
WeinaJi pushed a commit that referenced this pull request Oct 14, 2024
…; allow nonzero total current when simulating physical electrodes (#134)

## Context
NEURON reports SEClamp currents at different time points than all other
currents, thus giving inaccurate results in lfp calculations, in cases
where the SEClamp represents synaptic inputs.

This can be fixed by replacing the SEClamp with a new point process
mechanism that is identical, except for producing a NONSPECIFIC_CURRENT
instead of an ELECTRODE_CURRENT.

However, in cases where SEClamp or IClamp are used to represent physical
electrodes, rather than synaptic inputs, the currents should not be
included in the LFP calcualtion

## Scope
When a current_source or conductance_source is used, we check if the
alternative mod file, which is expected to be named
'MembraneCurrentSource.mod' or 'ConductanceSource.mod' is available. If
so, it is used, with nothing else changed, unless the key-value pair
"represents_physical_electrode" is set to True in the Stimulus Config
file. Otherwise the IClamp or SEClamp is used as before.

## Testing
Scientific test added to ensure that new mechanisms produce identical membrane voltages to the old ones

## Review
* [x] PR description is complete
* [x] Coding style (imports, function length, New functions, classes or
files) are good
* [x] Unit/Scientific test added
* [x] Updated Readme, in-code, developer documentation

---------

Co-authored-by: Jorge Blanco Alonso <jorge.blancoalonso@epfl.ch>
Co-authored-by: Jorge Blanco Alonso <41900536+jorblancoa@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants