nrnrandom.h: declare nrn*random* to mechanisms. by olupton · Pull Request #1755 · neuronsimulator/nrn

olupton · 2022-03-31T09:10:11Z

In this commit the declared prototypes take void*, which does not correspond to the definitions in ivocrand.cpp.
Additionally, nrn_set_random_sequence is declared to take an int second argument, despite its definition expecting a long.

The logic behind this change is that the declarations which are added to nrnrandom.h correspond to those that commonly occur in MOD files, even though they are not quite correct. In principle this means that adding these declarations to the headers that are implicitly included in translated MOD files will not introduce compilation failures in these models, because they are the same as the declarations included in the MOD files themselves.

nrnrandom.h: declare nrn*random* to mechanisms. #1755 (comment) is a counterexample, as there a naming clash in the MOD file means that the declaration in the MOD file is "even more wrong"

The declarations added in this PR correspond to the [[deprecated]] overloads of these methods that are included in #1597. The motivation for this change is that it makes it easier to update external models to be forward-compatible with #1597 ("NEURON 9") -- by removing declarations and adding casts -- while remaining compatible with master before that PR is merged (but after this PR is merged, "NEURON 8.2").

In this commit the declared prototypes take void*, which does not correspond to the definitions.

Note that this change is **optional**, this PR does not require that this (frequently copy/pasted) block is removed.

codecov-commenter · 2022-03-31T09:24:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.38%. Comparing base (331e750) to head (72105f5).
Report is 1792 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1755   +/-   ##
=======================================
  Coverage   45.38%   45.38%           
=======================================
  Files         551      551           
  Lines      113144   113144           
=======================================
  Hits        51355    51355           
  Misses      61789    61789

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

olupton · 2022-03-31T12:05:51Z

https://github.com/neuronsimulator/nrn-modeldb-ci/actions/runs/2070364088 showed that an earlier version of this PR (0369a15) only stopped one model from ModelDB from compiling.

The issue in that model is sgate.mod, which contains both

ASSIGNED {
  on (1)
  donotuse
  numtogo (1) : how many modulation cycles remain to be launched
  r (1)
}

and

VERBATIM
double nrn_random_pick(void* r);
void* nrn_random_arg(int argpos);
ENDVERBATIM

so in the translated code we get something like

#define r _p[9]
double nrn_random_pick(void* r);

which declares nrn_random_pick as taking one argument of type void**. This is doubly-wrong. The correct argument type (ivocrand.cpp) is Rand*, while the singly-wrong type declared in nrnrandom.h for compatibility is void*. The compilation error is that void* and void** are not the same.

Removing the r in double nrn_random_pick(void* r);, or replacing it with a name that is not a macro, would avoid this problem. But it illustrates the fragility of the approach of this PR.

olupton · 2022-03-31T13:06:16Z

https://github.com/neuronsimulator/nrn-modeldb-ci/actions/runs/2071195877 is running against the latest (72105f5) version of this PR. https://bbpgitlab.epfl.ch/hpc/coreneuron/-/pipelines/45215 is testing the same revision in the CoreNEURON pipeline, which includes BBP models.

olupton · 2022-03-31T14:20:44Z

The latest tests showed no further issues, and ModelDBRepository/187604#1 fixes the compilation failure noted above in sgate.mod.

pramodk

LGTM!

@nrnhines : FYI, this is a small step to make our life bit easier when we will go towards full MOD to CPP translation with #1597. An example like the one mentioned above will fail but this is acceptable IMO.

nrnhines

LGTM

* compatibility for master using neuronsimulator/nrn#1755 * Update README. * update readme mentioned tagged release Co-authored-by: Pramod S Kumbhar <pramod.s.kumbhar@gmail.com> Co-authored-by: Olli Lupton <oliver.lupton@epfl.ch>

* neuronsimulator/nrn#1755 lets us drop declarations. * other changes preparing for neuronsimulator/nrn#1597. Co-authored-by: Olli Lupton <oliver.lupton@epfl.ch>

* channel-benchmark!9 * HumanBrainProject/olfactory-bulb-3d#19, actually this PR merged into 2to3 branch. * neuronsimulator/nrntest#26 * neuronsimulator/reduced_dentate#5 * neuronsimulator/testcorenrn#10 * neuronsimulator/tqperf#11 * BlueBrain/CoreNeuron#792 * BlueBrain/CoreNeuron#793 The new versions should still support being translated to C code by NEURON versions with #1755, but should additionally support being translated to C++ code: see #1597. Co-authored-by: Olli Lupton <oliver.lupton@epfl.ch> Co-authored-by: Ioannis Magkanaris <iomagkanaris@gmail.com> Co-authored-by: Jorge Blanco Alonso <jorge.blancoalonso@epfl.ch>

olupton added 2 commits March 31, 2022 11:06

nrnrandom.h: declare nrn*random* to mechanisms.

d8ff141

In this commit the declared prototypes take void*, which does not correspond to the definitions.

netstim.mod: remove legacy declarations.

0369a15

Note that this change is **optional**, this PR does not require that this (frequently copy/pasted) block is removed.

Declare the Rand struct.

87c23f6

pramodk added a commit to HumanBrainProject/olfactory-bulb-3d that referenced this pull request Mar 31, 2022

compatibility for master using neuronsimulator/nrn#1755

71920e7

pramodk added a commit to HumanBrainProject/olfactory-bulb-3d that referenced this pull request Mar 31, 2022

compatibility for master using neuronsimulator/nrn#1755

4f641ce

nrn_set_random_sequence: declare int where we mean long.

72105f5

olupton mentioned this pull request Mar 31, 2022

Drop argument name in declaration. ModelDBRepository/187604#1

Merged

olupton marked this pull request as ready for review March 31, 2022 14:20

pramodk approved these changes Mar 31, 2022

View reviewed changes

pramodk added a commit to neuronsimulator/testcorenrn that referenced this pull request Mar 31, 2022

compatibility towards c++ using neuronsimulator/nrn#1755

6696621

pramodk mentioned this pull request Mar 31, 2022

compatibility towards c++ using nrn/pull/1755 neuronsimulator/testcorenrn#10

Merged

nrnhines approved these changes Mar 31, 2022

View reviewed changes

olupton merged commit a527c23 into master Mar 31, 2022

olupton deleted the olupton/mech-api-header-with-wrong-declarations branch March 31, 2022 21:03

This was referenced Apr 1, 2022

Drop obsolete declarations. neuronsimulator/nrntest#26

Merged

Make mod files compatible with C/C++ code generation in NEURON HumanBrainProject/olfactory-bulb-3d#19

Merged

olupton mentioned this pull request Apr 1, 2022

Compile NEURON mechanisms as C++ #1597

Merged

6 tasks

olupton mentioned this pull request Apr 7, 2022

api.hpp in place of mech/cfile/scoplib.h BlueBrain/nmodl#839

Closed

alexsavulescu mentioned this pull request Jun 28, 2022

NEURON 8.2.0 release #1879

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

nrnrandom.h: declare nrnrandom to mechanisms.#1755

nrnrandom.h: declare nrnrandom to mechanisms.#1755
olupton merged 4 commits into
masterfrom
olupton/mech-api-header-with-wrong-declarations

olupton commented Mar 31, 2022 •

edited

Loading

Uh oh!

codecov-commenter commented Mar 31, 2022 •

edited by codecov Bot

Loading

Uh oh!

olupton commented Mar 31, 2022 •

edited

Loading

Uh oh!

olupton commented Mar 31, 2022 •

edited

Loading

Uh oh!

olupton commented Mar 31, 2022

Uh oh!

pramodk left a comment

Uh oh!

nrnhines left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

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

Uh oh!

Uh oh!

codecov-commenter commented Mar 31, 2022 • edited by codecov Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

olupton commented Mar 31, 2022

Uh oh!

pramodk left a comment

Choose a reason for hiding this comment

Uh oh!

nrnhines left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

olupton commented Mar 31, 2022 •

edited

Loading

codecov-commenter commented Mar 31, 2022 •

edited by codecov Bot

Loading

olupton commented Mar 31, 2022 •

edited

Loading

olupton commented Mar 31, 2022 •

edited

Loading