neuronsimulator / nrn

NEURON Simulator
http://nrn.readthedocs.io
Other
376 stars 113 forks source link

Are top-LOCALs that are PARAMETERs valid? #2912

Closed 1uc closed 2 weeks ago

1uc commented 3 weeks ago

Can a variable be both LOCAL (at file scope) and mentioned in PARAMETER?

It compiles with NOCMODL/nrnivmodl, but the generated code is quite subtle and not exactly what I would have expected, but it isn't obviously broken either. Hence the question. Here's and example along with the generated code.

NEURON {
  SUFFIX top_local
}

PARAMETER {
  gbl = 3.0
}

LOCAL gbl

INITIAL {
  gbl = 2.0
}

NOMODL generates the following:

 #define _zgbl _thread[0].get<double*>()[0]
 /* declare global and static user variables */
#define gbl gbl_top_local
 double gbl = 3;

then registers the static variable for access via HOC:

 /* connect global user variables to hoc */
 static DoubScal hoc_scdoub[] = {
 {"gbl_top_local", &gbl_top_local},
 {0, 0}
};

however, the static variable isn't used on thread 0:

static void _thread_mem_init(Datum* _thread) {
   _thread[0] = {neuron::container::do_not_search, new double[1]{}};
 }

static void _thread_cleanup(Datum* _thread) {
   delete[] _thread[0].get<double*>();
 }

and later we use the copy for the thread variable:

static void _thread_mem_init(Datum* _thread) {
   _thread[0] = {neuron::container::do_not_search, new double[1]{}};
 }

static void _thread_cleanup(Datum* _thread) {
   delete[] _thread[0].get<double*>();
 }
1uc commented 3 weeks ago

I think this is invalid, because even if the top-LOCAL PARAMETER is read-only, nocmodl will create an independent thread-variable, instead of using the static double created for the PARAMETER of the same name.

pramodk commented 3 weeks ago

I wanted to say no but I will let @nrnhines confirm! From what I remember and have seen, I don't see why someone would want to make PARAMETER a LOCAL. Technically, yes? But from a model-writing perspective, it doesn't occurs as a natural thing (to me!).

(Just mentioning an additional aspect: in the past, when we have seen questions like this and if we have to decide whether we should forbid this and throw an error then what we have done is to check into ModelDB MOD file database to see if there is any such usages. Not saying we have to do it here! Just sharing how we have done some decisions like this)

Edit: By the way, I wrote my comment before seeing your above response.

nrnhines commented 3 weeks ago

PARAMETER and LOCAL seems like a duplicate declaration, I.e. error. But not quite as clearly so as if the name were duplicated in PARAMETER and ASSIGNED or STATE. In a sense PARAMETER + LOCAL could mean just a file scope instead of the broader scope of GLOBAL. It is certainly not a per instance variable. LOCAL was originally intended just to declare a file scope variable without making it available to the interpreter. With the advent of threads and gpu, it is not threadsafe and so has lost much of its value.

In the context of this issue, I would call it an error. But am open to other arguments.

1uc commented 2 weeks ago

I think it makes sense to consider it an error in the MOD file and a missing assert in nocmodl. Thank you clarifying.