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

Fix: handle edge case in synapse location calculation with syn_description #184

Merged
merged 3 commits into from
May 24, 2024

Conversation

ilkilic
Copy link
Contributor

@ilkilic ilkilic commented May 24, 2024

When retrieving the synapse location through syn_description, we did not properly handle cases where the location is 0. In NEURON, you cannot insert a synapse or any point process at the start (position 0) or end (position 1) of a segment if it needs an ion e.g. na, k, ca, cl etc. The section location must be strictly within the bounds (0, 1), excluding both endpoints.

@ilkilic
Copy link
Contributor Author

ilkilic commented May 24, 2024

also address #183

@ilkilic ilkilic requested a review from AurelienJaquier May 24, 2024 12:33
Copy link
Contributor

@AurelienJaquier AurelienJaquier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain what you did with the synapses? I am not familiar with synapse stuff

@ilkilic
Copy link
Contributor Author

ilkilic commented May 24, 2024

sure, I added a description above, let me know if it is not clear

When retrieving the synapse location through syn_description, we did not properly handle cases where the location is 0. In NEURON, you cannot insert a synapse or any point process at the start (position 0) or end (position 1) of a segment. The section location must be strictly within the bounds (0, 1), excluding both endpoints.

@AurelienJaquier
Copy link
Contributor

thanks for the explanations!

Copy link
Contributor

@AurelienJaquier AurelienJaquier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

@ilkilic ilkilic requested a review from darshanmandge May 24, 2024 14:24
@darshanmandge
Copy link
Contributor

Could you edit the description of the PR to say: "User cannot insert a point process located at positions 0 or 1 of a section if it needs an ion e.g. na, k, ca, cl etc. " ?

I checked with IClamp in NEURON. It works when inserted at the 0 end of a single compartment model as it uses a NONSPECIFIC_CURRENT i.

@ilkilic
Copy link
Contributor Author

ilkilic commented May 24, 2024

Thanks @darshanmandge for the precision, I have updated the text

@ilkilic ilkilic merged commit ebd0c1d into main May 24, 2024
10 checks passed
@ilkilic ilkilic deleted the fix-synapse-loc-edge-case branch May 24, 2024 14:44
@mgeplf
Copy link
Contributor

mgeplf commented May 27, 2024

@ilkilic > [...] insert a synapse or any point process at the start (position 0)

Do you know how neurodamus is handling this? I'm surprised that position 0 isn't allowed.

@darshanmandge
Copy link
Contributor

@ilkilic is away. I will comment to keep a record here. Based on our discussion internally, neurodamus handles this similar to the code in this PR in location_to_point at this line. The syanpses at 0 are moved to 0.0000001 and those at 1 are moved to 0.9999999 .

@mgeplf
Copy link
Contributor

mgeplf commented May 29, 2024

Thanks! We need to make sure these stay in sync.

@darshanmandge
Copy link
Contributor

Yes. true. I will also discuss with @anilbey to find if there is any other code in bluecellulab that we should check to keep in sync with neurodamus for maintaining reproducibility in results.

@anilbey
Copy link
Contributor

anilbey commented Jun 14, 2024

We have been using these regression tests for keeping them sync. For the new scientific features adding new regression on local SONATA networks will help.

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.

5 participants