Avoid use of ion variable in the CONSTANT block by pramodk · Pull Request #1955 · neuronsimulator/nrn

pramodk · 2022-08-17T21:38:26Z

certain mod files use read/write ion variables
from USEION statements in CONSTANT {} block

the generated code from such usage is not desired
I believe. For example, cdp_spiny.mod from ModelDB
model id 266799 has

NEURON {
	SUFFIX cdp4Nsp
 	USEION ca READ cao, cai, ica WRITE cai
 	RANGE ica_pmp,scale
  ...
}

ASSIGNED {
   diam      (um)
   ica       (mA/cm2)
   ica_pmp   (mA/cm2)
   parea     (um)     : pump area per unit length
   cai       (mM)
}

CONSTANT { cao = 2  (mM) }

In this example, the generated code doesn't set ion variable cao to 2
but declare a static variable cao with value 2 and that is not used
anywhere:

 #define ica _p[56]
 #define ica_columnindex 56
 #define parea _p[57]
 #define parea_columnindex 57
 #define cai _p[58]
 ...
 #define _ion_cao    *_ppvar[0]._pval
 #define _ion_cai    *_ppvar[1]._pval
 #define _ion_ica    *_ppvar[2]._pval
 #define _style_ca   *((int*)_ppvar[3]._pvoid)
 #define diam    *_ppvar[4]._pval
 ...
 static double cao = 2;
 ...
 static void nrn_state(NrnThread* _nt, _Memb_list* _ml, int _type) {

  ...
  cao = _ion_cao;
      cai = _ion_cai;
      ica = _ion_ica;
      cai = _ion_cai;
      ...
 }

Note that cao variable used/updated here is not ion variable but static one
defined in the file scope.

I believe this is not what user "expected" here.

As we see this pattern in multiple mod files (including glia__dbbs_mod_collection__cdp5__CAM_GoC.mod
mentioned in Testing & Fixing issues with DBBS-Lab's MOD file collection BlueBrain/nmodl#888), in this PR we disable
declaring ion variables as CONSTANT. With this PR, we will get an error like:
```
 $ ./bin/nocmodl /.../nmodldb/models/db/modeldb/266799/mod/cdp_spiny.mod
 ...
 cao used in USEION statement can not be re-declared in the CONSTANT block at line 299 in file /.../nmodldb/models/db/modeldb/266799/mod/cdp_spiny.mod
```

@nrnhines : do you agree that explicit error like above is better approach than silently generating incorrect/undesired code? 🤔

* certain mod files use read/write ion variables from USEION statements in CONSTANT {} block * the generated code from such usage is not desired I believe. For example, cdp_spiny.mod from ModelDB model id 266799 has ```console NEURON { SUFFIX cdp4Nsp USEION ca READ cao, cai, ica WRITE cai RANGE ica_pmp,scale ... } ASSIGNED { diam (um) ica (mA/cm2) ica_pmp (mA/cm2) parea (um) : pump area per unit length cai (mM) } CONSTANT { cao = 2 (mM) } ``` In this example, the generated code doesn't set ion variable `cao` to 2 but declare a static variable `cao` with value 2 and that is not used anywhere: ```cpp #define ica _p[56] #define ica_columnindex 56 #define parea _p[57] #define parea_columnindex 57 #define cai _p[58] ... #define _ion_cao *_ppvar[0]._pval #define _ion_cai *_ppvar[1]._pval #define _ion_ica *_ppvar[2]._pval #define _style_ca *((int*)_ppvar[3]._pvoid) #define diam *_ppvar[4]._pval ... static double cao = 2; ... static void nrn_state(NrnThread* _nt, _Memb_list* _ml, int _type) { ... cao = _ion_cao; cai = _ion_cai; ica = _ion_ica; cai = _ion_cai; ... } ``` Note that `cao` variable used/updated here is not ion variable but static one defined in the file scope. I believe this is not what user "expected" here. * As we see this pattern in multiple mod files (including glia__dbbs_mod_collection__cdp5__CAM_GoC.mod mentioned in BlueBrain/nmodl#888), in this PR we disable declaring ion variables as CONSTANT. With this PR, we will get an error like: ```console $ ./bin/nocmodl /.../nmodldb/models/db/modeldb/266799/mod/cdp_spiny.mod ... cao used in USEION statement can not be re-declared in the CONSTANT block at line 299 in file /.../nmodldb/models/db/modeldb/266799/mod/cdp_spiny.mod ```

codecov-commenter · 2022-08-17T22:09:51Z

Codecov Report

Merging #1955 (e4d57da) into master (1b76917) will increase coverage by 0.00%.
The diff coverage is 93.33%.

@@           Coverage Diff           @@
##           master    #1955   +/-   ##
=======================================
  Coverage   46.50%   46.50%           
=======================================
  Files         526      526           
  Lines      119216   119231   +15     
=======================================
+ Hits        55439    55453   +14     
- Misses      63777    63778    +1

Impacted Files	Coverage Δ
src/nmodl/nocpout.cpp	`93.25% <92.85%> (-0.01%)`	⬇️
src/nmodl/modl.cpp	`67.92% <100.00%> (+0.30%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

azure-pipelines · 2022-08-17T22:17:16Z

✔️ 690ccab -> Azure artifacts URL

* Similar to neuronsimulator/nrn#1955 * Add semantic analysis check to avoid declaring ion variables in CONSTANT {} block * Addresses first point in #888 MOD file is glia__dbbs_mod_collection__cdp5__CAM_GoC.mod. * Updated test

pramodk · 2022-08-25T21:10:58Z

@nrnhines : do you agree that explicit error like above is better approach than silently generating incorrect/undesired code? 🤔

@nrnhines : what do you think?

nrnhines · 2022-08-25T21:14:58Z

The explicit error is better. Thanks.

pramodk · 2022-08-25T21:19:26Z

ok good! I think there might be some mod files in the ModelDB. Before merging this PR, I can quickly check this in ModelDB and maybe fix this.

azure-pipelines · 2022-09-06T11:16:17Z

✔️ 0462052 -> Azure artifacts URL

USEION variable can not be initialized in CONSTANT block. See neuronsimulator/nrn#1955.

USEION variable can not be initialized in a CONSTANT block. See neuronsimulator/nrn#1955.

pramodk · 2022-09-07T09:49:51Z

There are 3 models with 8 mod files where this pattern was used. It seems like all those mod files are same / similar and originated from the same implementation (filename with cdp*.mod).

I have created PRs here:

Updated MOD file to fix the issue with the upcoming NEURON 9.0 release ModelDBRepository/244679#1
Updated MOD file to fix the issue with the upcoming NEURON 9.0 release ModelDBRepository/265584#1
Updated MOD file to fix the issue with the upcoming NEURON 9.0 release ModelDBRepository/243446#1

azure-pipelines · 2022-09-07T10:36:38Z

✔️ 4afde67 -> Azure artifacts URL

olupton · 2022-09-07T14:56:05Z

There are 3 models with 8 mod files where this pattern was used. It seems like all those mod files are same / similar and originated from the same implementation (filename with cdp*.mod).

I have created PRs here:
* ModelDBRepository/244679#1
* ModelDBRepository/265584#1
* ModelDBRepository/243446#1

I approved these, I believe that @ramcdougal needs to do something manual to make sure that the changes are propagated properly after the PRs are merged.

* Similar to neuronsimulator/nrn#1955 * Add semantic analysis check to avoid declaring ion variables in CONSTANT {} block * Addresses first point in #888 MOD file is glia__dbbs_mod_collection__cdp5__CAM_GoC.mod. * Updated test

* Similar to neuronsimulator/nrn#1955, add semantic analysis check to avoid declaring ion variables in CONSTANT {} block * Addresses first point in #888 MOD file is glia__dbbs_mod_collection__cdp5__CAM_GoC.mod. * Updated test

…ase) * Setting ionic variable like `cao`` in CONSTANT block had no effect See neuronsimulator/nrn#1955 * This will be an error in future neuron v9 and with the latest neuron-nightly pypi package. * As setting cao from MOD file had no effect, simply uncomment this (?)

alexsavulescu · 2022-09-12T09:08:58Z

@pramodk looking at the modeldb-ci nightly there is also 138382. I commented out the cao CONSTANT assignment but now I see:

cdp3.cpp:2144:3: error: ‘cao’ was not declared in this scope; did you mean ‘ca0’?
 2144 |   cao = _ion_cao;
      |   ^~~
      |   ca0
cdp5.cpp: In function ‘void _ode_spec(NrnThread*, Memb_list*, int)’:
cdp5.cpp:1611:3: error: ‘cao’ was not declared in this scope; did you mean ‘ca0’?
....

See neuronsimulator/nrn#1955.

pramodk · 2022-09-12T09:35:43Z

@pramodk looking at the modeldb-ci nightly there is also 138382. I commented out the cao CONSTANT assignment but now I see:

Should be fixed by ModelDBRepository/138382#1

alexsavulescu · 2022-09-12T09:45:30Z

thanks. updated to fix cdp5.mod in the same manner

#1) * Updated MOD file to fix the issue with the upcoming NEURON 9.0 release USEION variable can not be initialized in a CONSTANT block. See neuronsimulator/nrn#1955. * fix missing declaration of cao

…pcoming neuron release) (#6) * Ion variables can not be set via CONSTANT block (upcoming neuron release) * Setting ionic variable like `cao`` in CONSTANT block had no effect See neuronsimulator/nrn#1955 * This will be an error in future neuron v9 and with the latest neuron-nightly pypi package. * As setting cao from MOD file had no effect, simply uncomment this (?) * fix missing declaration of cao and remove CONSTANT block

* USEION variable can not be initialized in a CONSTANT block. See neuronsimulator/nrn#1955. * cdp5.mod --------- Co-authored-by: Alexandru Săvulescu <alexandru.savulescu@epfl.ch>

…rain/nmodl#908) * Similar to #1955, add semantic analysis check to avoid declaring ion variables in CONSTANT {} block * Addresses first point in BlueBrain/nmodl#888 MOD file is glia__dbbs_mod_collection__cdp5__CAM_GoC.mod. * Updated test NMODL Repo SHA: BlueBrain/nmodl@af7c772

pramodk added enhancement nmodl labels Aug 17, 2022

pramodk requested review from alkino and nrnhines August 17, 2022 21:38

make clang-format happy

690ccab

pramodk mentioned this pull request Aug 17, 2022

Semantic analysis check for USEION variables in CONSTANT block BlueBrain/nmodl#908

Merged

Merge branch 'master' into pramodk/ion-var-as-constant

e4d57da

nrnhines approved these changes Aug 25, 2022

View reviewed changes

Merge branch 'master' into pramodk/ion-var-as-constant

0462052

pramodk added a commit to ModelDBRepository/244679 that referenced this pull request Sep 7, 2022

Updated MOD file to fix the issue with the upcoming NEURON 9.0 release

7db7508

USEION variable can not be initialized in CONSTANT block. See neuronsimulator/nrn#1955.

pramodk added a commit to ModelDBRepository/244679 that referenced this pull request Sep 7, 2022

Updated MOD file to fix the issue with the upcoming NEURON 9.0 release

6466bcb

USEION variable can not be initialized in a CONSTANT block. See neuronsimulator/nrn#1955.

pramodk mentioned this pull request Sep 7, 2022

Updated MOD file to fix the issue with the upcoming NEURON 9.0 release ModelDBRepository/244679#1

Merged

pramodk added a commit to ModelDBRepository/265584 that referenced this pull request Sep 7, 2022

Updated MOD file to fix the issue with the upcoming NEURON 9.0 release

82a7539

USEION variable can not be initialized in a CONSTANT block. See neuronsimulator/nrn#1955.

pramodk mentioned this pull request Sep 7, 2022

Updated MOD file to fix the issue with the upcoming NEURON 9.0 release ModelDBRepository/265584#1

Merged

pramodk added a commit to ModelDBRepository/243446 that referenced this pull request Sep 7, 2022

Updated MOD file to fix the issue with the upcoming NEURON 9.0 release

fa07bc9

USEION variable can not be initialized in a CONSTANT block. See neuronsimulator/nrn#1955.

pramodk added a commit to ModelDBRepository/243446 that referenced this pull request Sep 7, 2022

Updated MOD file to fix the issue with the upcoming NEURON 9.0 release

b6aa82f

USEION variable can not be initialized in a CONSTANT block. See neuronsimulator/nrn#1955.

pramodk mentioned this pull request Sep 7, 2022

Updated MOD file to fix the issue with the upcoming NEURON 9.0 release ModelDBRepository/243446#1

Merged

pramodk mentioned this pull request Sep 7, 2022

Try to declare dependencies on libcorenrnmech.so properly BlueBrain/CoreNeuron#855

Merged

pramodk added 2 commits September 7, 2022 11:58

Merge branch 'master' into pramodk/ion-var-as-constant

1ea8164

Use --oversubscribe even with 2 ranks

4afde67

pramodk merged commit e8fe008 into master Sep 7, 2022

pramodk deleted the pramodk/ion-var-as-constant branch September 7, 2022 11:56

pramodk mentioned this pull request Sep 9, 2022

Ion variables can not be set via CONSTANT block (compatibility with upcoming neuron release) dbbs-lab/dbbs-mod-collection#6

Merged

pramodk added a commit to ModelDBRepository/138382 that referenced this pull request Sep 12, 2022

USEION variable can not be initialized in a CONSTANT block.

c3023ee

See neuronsimulator/nrn#1955.

pramodk mentioned this pull request Sep 12, 2022

USEION variable can not be initialized in a CONSTANT block. ModelDBRepository/138382#1

Merged

JCGoran mentioned this pull request Sep 1, 2025

Fixes for NEURON 9 ModelDBRepository/266806#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Avoid use of ion variable in the CONSTANT block#1955

Avoid use of ion variable in the CONSTANT block#1955
pramodk merged 6 commits into
masterfrom
pramodk/ion-var-as-constant

pramodk commented Aug 17, 2022

Uh oh!

codecov-commenter commented Aug 17, 2022 •

edited

Loading

Uh oh!

azure-pipelines Bot commented Aug 17, 2022

Uh oh!

pramodk commented Aug 25, 2022

Uh oh!

nrnhines commented Aug 25, 2022

Uh oh!

pramodk commented Aug 25, 2022

Uh oh!

azure-pipelines Bot commented Sep 6, 2022

Uh oh!

pramodk commented Sep 7, 2022

Uh oh!

azure-pipelines Bot commented Sep 7, 2022

Uh oh!

olupton commented Sep 7, 2022 •

edited

Loading

Uh oh!

alexsavulescu commented Sep 12, 2022

Uh oh!

pramodk commented Sep 12, 2022

Uh oh!

alexsavulescu commented Sep 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

pramodk commented Aug 17, 2022

Uh oh!

codecov-commenter commented Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

azure-pipelines Bot commented Aug 17, 2022

Uh oh!

pramodk commented Aug 25, 2022

Uh oh!

nrnhines commented Aug 25, 2022

Uh oh!

pramodk commented Aug 25, 2022

Uh oh!

azure-pipelines Bot commented Sep 6, 2022

Uh oh!

pramodk commented Sep 7, 2022

Uh oh!

azure-pipelines Bot commented Sep 7, 2022

Uh oh!

olupton commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexsavulescu commented Sep 12, 2022

Uh oh!

pramodk commented Sep 12, 2022

Uh oh!

alexsavulescu commented Sep 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Aug 17, 2022 •

edited

Loading

olupton commented Sep 7, 2022 •

edited

Loading