Code generation fixes for array variables in TABLE statement by pramodk · Pull Request #924 · BlueBrain/nmodl

pramodk · 2022-09-04T21:27:31Z

table statements can have array variables. Until now only
scalar variables were supported in code generation.
we can check symbol table to find out if the variable is
an array and it's length.
similar to mod2c implementation, generate code for array
variable assignments:
https://github.com/BlueBrain/mod2c/blob/469c74dc7d96bbc5a06a42696422154b4cd2ce28/src/mod2c_core/parsact.c#L942
with this, glia__dbbs_mod_collection__Cav2_3__0.mod from Testing & Fixing issues with DBBS-Lab's MOD file collection #888
compiles

* table statements can have array variables. Until now only scalar variables were supported in code generation. * we can check symbol table to find out if the variable is an array and it's length. * similar to mod2c implementation, generate code for array variable assignments: https://github.com/BlueBrain/mod2c/blob/469c74dc7d96bbc5a06a42696422154b4cd2ce28/src/mod2c_core/parsact.c#L942 * with this, `glia__dbbs_mod_collection__Cav2_3__0.mod` from #888 compiles

pramodk · 2022-09-04T21:28:54Z

@alkino: for array variables, this is how one could use symbol table to determine how array variables needs to be printed. MOD2C does similar thing using symbol table.

bbpbuildbot · 2022-09-04T22:00:24Z

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

Status and direct links:

codecov-commenter · 2022-09-05T02:39:21Z

Codecov Report

Merging #924 (bbbb0d1) into master (60249f1) will decrease coverage by 0.00%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #924      +/-   ##
==========================================
- Coverage   61.46%   61.46%   -0.01%     
==========================================
  Files         194      194              
  Lines       28457    28481      +24     
==========================================
+ Hits        17492    17506      +14     
- Misses      10965    10975      +10

Impacted Files	Coverage Δ
src/codegen/codegen_c_visitor.hpp	`91.48% <ø> (ø)`
src/codegen/codegen_c_visitor.cpp	`69.33% <66.66%> (-0.10%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

alkino · 2022-09-05T08:16:15Z

We should try with range and not range variable

pramodk · 2022-09-05T08:24:19Z

We should try with range and not range variable

You mean for testing? (i.e. using GLOBAL variable in TABLE)

pramodk · 2022-09-05T10:06:07Z

@alkino: I tested with GLOBAL variable as well:

 NEURON {
 SUFFIX glia__dbbs_mod_collection__Cav2_3__0
     THREADSAFE
     USEION ca READ eca WRITE ica
     RANGE gcabar, m, h, g, gmax
     RANGE inf, tau
     GLOBAL xx
 }

 UNITS {
     (mA) = (milliamp)
     (mV) = (millivolt)
 }

 PARAMETER {              : parameters that can be entered when function is called in cell-setup
     v             (mV)
     celsius
     gcabar = 0    (mho/cm2) : initialized conductance
     xx = 1
 }

....

 PROCEDURE mhn(v (mV)) {
     TABLE inf, tau, xx DEPEND celsius FROM -100 TO 100 WITH 200
     FROM i=0 TO 1 {
         tau[i] = vartau(v,i)
         inf[i] = varss(v,i) + xx
     }
 }

And generated code is reasonable / same as mod2c:

     /** all global variables */
     struct glia__dbbs_mod_collection__Cav2_3__0_Store {
         int ca_type{};
         double m0{};
         double h0{};
         int reset{};
         int mech_type{};
         double xx{1};
         int slist1[2]{7, 8};
         int dlist1[2]{9, 10};
         double usetable{1};
         double tmin_mhn{};
         double mfac_mhn{};
         double t_inf[201]{};
         double t_tau[201]{};
         double t_xx[201]{};
     };

     /** all mechanism instance variables and global variables */
     struct glia__dbbs_mod_collection__Cav2_3__0_Instance  {
...
         glia__dbbs_mod_collection__Cav2_3__0_Store* global{&glia__dbbs_mod_collection__Cav2_3__0_global};
     };

...

     inline int mhn_glia__dbbs_mod_collection__Cav2_3__0(int id, int pnodecount, glia__dbbs_mod_collection__Cav2_3__0_Instance* inst, double* data, const Datum* indexes,           ThreadDatum* thread, NrnThread* nt, double v, double arg_v){
         if (inst->global->usetable == 0) {
             f_mhn_glia__dbbs_mod_collection__Cav2_3__0(id, pnodecount, inst, data, indexes, thread, nt, v, arg_v);
             return 0;
         }
         double xi = inst->global->mfac_mhn * (arg_v - inst->global->tmin_mhn);
         if (isnan(xi)) {
             (inst->inf+id*2)[0] = xi;
             (inst->inf+id*2)[1] = xi;
             (inst->tau+id*2)[0] = xi;
             (inst->tau+id*2)[1] = xi;
             inst->global->xx = xi;
             return 0;
         }
         if (xi <= 0. || xi >= 200.) {
             int index = (xi <= 0.) ? 0 : 200;
             (inst->inf+id*2)[0] = inst->global->t_inf[index];
             (inst->inf+id*2)[1] = inst->global->t_inf[index];
             (inst->tau+id*2)[0] = inst->global->t_tau[index];
             (inst->tau+id*2)[1] = inst->global->t_tau[index];
             inst->global->xx = inst->global->t_xx[index];
             return 0;
         }
         int i = int(xi);
         double theta = xi - double(i);
         (inst->inf+id*2)[0] = inst->global->t_inf[i] + theta*(inst->global->t_inf[i+1]-inst->global->t_inf[i]);
         (inst->inf+id*2)[1] = inst->global->t_inf[i] + theta*(inst->global->t_inf[i+1]-inst->global->t_inf[i]);
         (inst->tau+id*2)[0] = inst->global->t_tau[i] + theta*(inst->global->t_tau[i+1]-inst->global->t_tau[i]);
         (inst->tau+id*2)[1] = inst->global->t_tau[i] + theta*(inst->global->t_tau[i+1]-inst->global->t_tau[i]);
         inst->global->xx = inst->global->t_xx[i] + theta*(inst->global->t_xx[i+1]-inst->global->t_xx[i]);
         return 0;
     }

we don't have a good (integration) test but we will handle this once we get all issues from #888 covered (and some work with @iomaganaris about testing/code coverage aspects).

iomaganaris · 2022-09-06T11:16:20Z

@alkino: I tested with GLOBAL variable as well:

 NEURON {
 SUFFIX glia__dbbs_mod_collection__Cav2_3__0
     THREADSAFE
     USEION ca READ eca WRITE ica
     RANGE gcabar, m, h, g, gmax
     RANGE inf, tau
     GLOBAL xx
 }

 UNITS {
     (mA) = (milliamp)
     (mV) = (millivolt)
 }

 PARAMETER {              : parameters that can be entered when function is called in cell-setup
     v             (mV)
     celsius
     gcabar = 0    (mho/cm2) : initialized conductance
     xx = 1
 }

....

 PROCEDURE mhn(v (mV)) {
     TABLE inf, tau, xx DEPEND celsius FROM -100 TO 100 WITH 200
     FROM i=0 TO 1 {
         tau[i] = vartau(v,i)
         inf[i] = varss(v,i) + xx
     }
 }

And generated code is reasonable / same as mod2c:

     /** all global variables */
     struct glia__dbbs_mod_collection__Cav2_3__0_Store {
         int ca_type{};
         double m0{};
         double h0{};
         int reset{};
         int mech_type{};
         double xx{1};
         int slist1[2]{7, 8};
         int dlist1[2]{9, 10};
         double usetable{1};
         double tmin_mhn{};
         double mfac_mhn{};
         double t_inf[201]{};
         double t_tau[201]{};
         double t_xx[201]{};
     };

     /** all mechanism instance variables and global variables */
     struct glia__dbbs_mod_collection__Cav2_3__0_Instance  {
...
         glia__dbbs_mod_collection__Cav2_3__0_Store* global{&glia__dbbs_mod_collection__Cav2_3__0_global};
     };

...

     inline int mhn_glia__dbbs_mod_collection__Cav2_3__0(int id, int pnodecount, glia__dbbs_mod_collection__Cav2_3__0_Instance* inst, double* data, const Datum* indexes,           ThreadDatum* thread, NrnThread* nt, double v, double arg_v){
         if (inst->global->usetable == 0) {
             f_mhn_glia__dbbs_mod_collection__Cav2_3__0(id, pnodecount, inst, data, indexes, thread, nt, v, arg_v);
             return 0;
         }
         double xi = inst->global->mfac_mhn * (arg_v - inst->global->tmin_mhn);
         if (isnan(xi)) {
             (inst->inf+id*2)[0] = xi;
             (inst->inf+id*2)[1] = xi;
             (inst->tau+id*2)[0] = xi;
             (inst->tau+id*2)[1] = xi;
             inst->global->xx = xi;
             return 0;
         }
         if (xi <= 0. || xi >= 200.) {
             int index = (xi <= 0.) ? 0 : 200;
             (inst->inf+id*2)[0] = inst->global->t_inf[index];
             (inst->inf+id*2)[1] = inst->global->t_inf[index];
             (inst->tau+id*2)[0] = inst->global->t_tau[index];
             (inst->tau+id*2)[1] = inst->global->t_tau[index];
             inst->global->xx = inst->global->t_xx[index];
             return 0;
         }
         int i = int(xi);
         double theta = xi - double(i);
         (inst->inf+id*2)[0] = inst->global->t_inf[i] + theta*(inst->global->t_inf[i+1]-inst->global->t_inf[i]);
         (inst->inf+id*2)[1] = inst->global->t_inf[i] + theta*(inst->global->t_inf[i+1]-inst->global->t_inf[i]);
         (inst->tau+id*2)[0] = inst->global->t_tau[i] + theta*(inst->global->t_tau[i+1]-inst->global->t_tau[i]);
         (inst->tau+id*2)[1] = inst->global->t_tau[i] + theta*(inst->global->t_tau[i+1]-inst->global->t_tau[i]);
         inst->global->xx = inst->global->t_xx[i] + theta*(inst->global->t_xx[i+1]-inst->global->t_xx[i]);
         return 0;
     }

we don't have a good (integration) test but we will handle this once we get all issues from #888 covered (and some work with @iomaganaris about testing/code coverage aspects).

Maybe not a bad idea to add a small example like the above to the unit tests of codegen c visitor in https://github.com/BlueBrain/nmodl/blob/master/test/unit/codegen/codegen_c_visitor.cpp?

bbpbuildbot · 2022-09-06T16:31:33Z

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

Status and direct links:

bbpbuildbot · 2022-09-06T17:30:25Z

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

Status and direct links:

pramodk · 2022-09-06T19:13:04Z

Maybe not a bad idea to add a small example like the above to the unit tests of codegen c visitor in https://github.com/BlueBrain/nmodl/blob/master/test/unit/codegen/codegen_c_visitor.cpp?

@iomaganaris : done in ec56905

iomaganaris

LGTM 👍

…in/nmodl#924) * Table statements can have array variables. Until now only scalar variables were supported in code generation. * We can check symbol table to find out if the variable is an array and it's length. * Similar to mod2c implementation, generate code for array variable assignments: https://github.com/BlueBrain/mod2c/blob/469c74dc7d96bbc5a06a42696422154b4cd2ce28/src/mod2c_core/parsact.c#L942 * with this, `glia__dbbs_mod_collection__Cav2_3__0.mod` from BlueBrain/nmodl#888 compiles * Add test and fix the bug for array variables allocation and access NMODL Repo SHA: BlueBrain/nmodl@6a59caa

pramodk requested a review from alkino September 5, 2022 07:52

pramodk mentioned this pull request Sep 6, 2022

Testing & Fixing issues with DBBS-Lab's MOD file collection #888

Open

6 tasks

Add test and fix the bug for array variables allocation and access

ec56905

do not need sympy visitor in codegen test

2000d24

iomaganaris approved these changes Sep 7, 2022

View reviewed changes

pramodk merged commit 6a59caa into master Sep 7, 2022

pramodk deleted the pramodk/table-var-as-array branch September 7, 2022 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Code generation fixes for array variables in TABLE statement#924

Code generation fixes for array variables in TABLE statement#924
pramodk merged 3 commits into
masterfrom
pramodk/table-var-as-array

pramodk commented Sep 4, 2022

Uh oh!

pramodk commented Sep 4, 2022

Uh oh!

bbpbuildbot commented Sep 4, 2022

Uh oh!

codecov-commenter commented Sep 5, 2022

Uh oh!

alkino commented Sep 5, 2022

Uh oh!

pramodk commented Sep 5, 2022

Uh oh!

pramodk commented Sep 5, 2022

Uh oh!

iomaganaris commented Sep 6, 2022

Uh oh!

bbpbuildbot commented Sep 6, 2022

Uh oh!

bbpbuildbot commented Sep 6, 2022

Uh oh!

pramodk commented Sep 6, 2022

Uh oh!

iomaganaris left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

pramodk commented Sep 4, 2022

Uh oh!

pramodk commented Sep 4, 2022

Uh oh!

bbpbuildbot commented Sep 4, 2022

Uh oh!

codecov-commenter commented Sep 5, 2022

Codecov Report

Uh oh!

alkino commented Sep 5, 2022

Uh oh!

pramodk commented Sep 5, 2022

Uh oh!

pramodk commented Sep 5, 2022

Uh oh!

iomaganaris commented Sep 6, 2022

Uh oh!

bbpbuildbot commented Sep 6, 2022

Uh oh!

bbpbuildbot commented Sep 6, 2022

Uh oh!

pramodk commented Sep 6, 2022

Uh oh!

iomaganaris left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants