Skip to content
This repository was archived by the owner on Mar 25, 2025. It is now read-only.

Correctly set conductances when using NONSPECIFIC_CURRENT#1218

Merged
1uc merged 12 commits into
masterfrom
jelic/conductances
Apr 8, 2024
Merged

Correctly set conductances when using NONSPECIFIC_CURRENT#1218
1uc merged 12 commits into
masterfrom
jelic/conductances

Conversation

@JCGoran

@JCGoran JCGoran commented Mar 14, 2024

Copy link
Copy Markdown
Contributor

Attempt at fixing #1190

@bbpbuildbot

This comment has been minimized.

Sign in to view
@JCGoran JCGoran force-pushed the jelic/conductances branch from 3f50659 to 7d4b887 Compare March 14, 2024 11:02
@codecov-commenter

codecov-commenter commented Mar 14, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 62.50000% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 86.52%. Comparing base (2edf9ac) to head (d708763).

❗ Current head d708763 differs from pull request most recent head 05a649b. Consider uploading reports for the commit 05a649b to get more accurate results

Files Patch % Lines
src/codegen/codegen_neuron_cpp_visitor.cpp 62.50% 9 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1218      +/-   ##
==========================================
- Coverage   86.58%   86.52%   -0.06%     
==========================================
  Files         176      176              
  Lines       13045    13071      +26     
==========================================
+ Hits        11295    11310      +15     
- Misses       1750     1761      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bbpbuildbot

This comment has been minimized.

Sign in to view
@JCGoran JCGoran added the NEURON codegen Work toward NEURON code generation label Mar 15, 2024
Goran Jelic-Cizmek added 4 commits March 15, 2024 15:15
Fix for other models
98464be 
TODO somehow transmit information to the jacobian maybe? Otherwise, this
works...
Revert "Fix for other models"
c466052 
This reverts commit 98464be.
Generalize setting conductances
77b1819
Update references commit
abea623
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
JCGoran
JCGoran commented Mar 18, 2024
View reviewed changes
Comment thread src/codegen/codegen_neuron_cpp_visitor.cpp Outdated
@JCGoran

JCGoran commented Mar 18, 2024

Copy link
Copy Markdown
Contributor Author

Associated PR for updating references: BlueBrain/nmodl-references#7

@JCGoran JCGoran marked this pull request as ready for review March 18, 2024 08:28
@JCGoran JCGoran requested a review from 1uc March 18, 2024 08:28
@JCGoran

JCGoran commented Mar 18, 2024

Copy link
Copy Markdown
Contributor Author

Also a question for the reviewer: should I add a test for this feature?

@JCGoran JCGoran linked an issue Mar 18, 2024 that may be closed by this pull request
Review setting conductances when generating code for NEURON. #1190
Closed
@JCGoran JCGoran changed the title Correctly set conductances when using NONSPECIFIC_CURRENT for NEURON codegen Correctly set conductances when using NONSPECIFIC_CURRENT Mar 18, 2024
@bbpbuildbot

This comment has been minimized.

Sign in to view
@JCGoran

JCGoran commented Mar 27, 2024

Copy link
Copy Markdown
Contributor Author

For future reference, when using the nonspecific_current mod file, the call stack of NEURON+NOCMODL looks a bit like this (relevant function calls and assignment statements are pointed out):

nrn_fixed_step_thread (or nrn_finitialize if looking at the first run)
  setup_tree_matrix
    nrn_rhs
      vec_rhs[i] = 0. # zero out RHS, L405
      nrn_cur_[MECHANISM] # updates RHS, but not conductances
      dv = vec_v[pi] - vec_v[i]
      vec_rhs[i] -= vec_b[i] * dv # updates RHS (axial?)
      vec_rhs[pi] += vec_a[i] * dv # also updates RHS (axial?)
    nrn_lhs
      vec_d[i] = 0. # zero out diag
      nrn_jacob_[MECHANISM] # we set conductances here originally (g_unused)
        _vec_d[_ni[_iml]] += _g
      nrn_cap_jacob
        vec_d[ni[i]] += cfac * ml_cache.fpfield<cm_index>(i) # updates diagonal
      vec_d[i] -= vec_b[i]
      vec_d[_nt->_v_parent_index[i]] -= vec_a[i]
  nrn_solve
    triang
      p = vec_a[i] / vec_d[i]
      pi = _nt->_v_parent_index[i]
      vec_d[pi] -= p * vec_b[i]
      vec_rhs[pi] -= p * vec_rhs[i]
    bksub
      vec_rhs[i] /= vec_d[i] # diagonal part
      vec_rhs[i] -= vec_b[i] * vec_rhs[_nt->_v_parent_index[i]] # axial part
      vec_rhs[i] /= vec_d[i] # second part using the diagonal part
  nrn_update_voltage
    vec_v[i] += vec_rhs[i] # update voltage from RHS
    nrn_capacity_current
      ml_cache.fpfield<i_cap_index>(i) = cfac * ml_cache.fpfield<cm_index>(i) *
        vec_rhs[ni[i]] # something using RHS
  nrn_fixed_step_lastpart
    nrn_state_[MECHANISM] # does nothing in our case

If we wanted to inline the setting of the diagonal/conductances (that is, place them in nrn_cur_[MECHANISM]), we would need to alter NEURON in the following way:

  • move zeroing out of diagonal from nrn_lhs to nrn_rhs
  • move nrn_cap_jacob from nrn_lhs to nrn_rhs

Unfortunately, this would require modifying code generated by NOCMODL as well, so it seems that for now we need to keep using nrn_jac_[MECHANISM] for setting the diagonal properly.

@bbpbuildbot

bbpbuildbot commented Apr 2, 2024

Copy link
Copy Markdown
Collaborator

Logfiles from GitLab pipeline #203066 (:white_check_mark:) have been uploaded here!

Status and direct links:

@JCGoran JCGoran mentioned this pull request Apr 2, 2024
Closed
@JCGoran JCGoran mentioned this pull request Apr 2, 2024
Merged
JCGoran
JCGoran commented Apr 2, 2024
View reviewed changes
Comment thread src/codegen/codegen_neuron_cpp_visitor.cpp
@1uc 1uc merged commit 593f38e into master Apr 8, 2024
@1uc 1uc deleted the jelic/conductances branch April 8, 2024 09:52
JCGoran added a commit to neuronsimulator/nrn that referenced this pull request Mar 12, 2025
@JCGoran
Set conductances when using NONSPECIFIC_CURRENT (BlueBrain/nmodl#1218)
cb5ad5d 
NMODL Repo SHA: BlueBrain/nmodl@593f38e
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@1uc 1uc 1uc approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

NEURON codegen Work toward NEURON code generation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Review setting conductances when generating code for NEURON.

4 participants

@JCGoran @bbpbuildbot @codecov-commenter @1uc