Skip to content

New NMODL keyword for random variables. NEURON { RANDOM ranvar }#2627

Merged
pramodk merged 95 commits into
masterfrom
hines/nmodlrandom-simplified
Feb 8, 2024
Merged

New NMODL keyword for random variables. NEURON { RANDOM ranvar }#2627
pramodk merged 95 commits into
masterfrom
hines/nmodlrandom-simplified

Conversation

@nrnhines

@nrnhines nrnhines commented Dec 6, 2023

Copy link
Copy Markdown
Member

A simplified alternative to #2597
See the documentation as part of this PR. Basically the random variable specifies only the generator and its properties, not the distribution. Default nrnran123_State properties are unique per instance and can be set and get by HOC or Python.

See BlueBrain/nmodl#1125

The new hoc syntax increases SHIFT/REDUCE conflicts from 96 to 108

image

CI_BRANCHES:NMODL_BRANCH=master,

pramodk and others added 26 commits November 20, 2023 15:58
cp ../external/coding-conventions/cpp/clang-format-10 ../.clang-format
clang-format-11 -i ./src/nrnoc/netstim.cpp
cp ../external/coding-conventions/cpp/clang-format-10 ../.clang-format
clang-format-11 -i ./src/nrnoc/netstim.cpp
1. variables are properly declared and registered
2. necessary functions are declared, defined and registered
3. able to call the functions from python world
4. add unit test to the same
- added src/nrnoc/netstim_rng.mod so that existing tests
  can work and we have a "cleaned" MOD file for RANDOM
- have basic support implemented for setseq, init, sample
  methods with RANDOM variable
- RANDOM variables are registered with "random" semantics
  but they are now treated as POINTER variables
- revert src/nrnoc/netstim.mod to origin/master
- remove temporary generated file cmake-build-debug/src/nrnoc/netstim.cpp
(but not getting error when owning handle goes out of existence)
@nrnhines nrnhines requested review from ohm314 and pramodk December 6, 2023 13:18

@pramodk pramodk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks OK. I will copy the comments that I just posted over Slack so that we have those here on the review.

Comment thread docs/python/modelspec/programmatic/mechanisms/nmodl2.rst Outdated
Comment thread docs/python/programming/math/random.rst
Comment thread src/nmodl/extdef_rand.h Outdated
Comment thread src/nmodl/nmodlfunc.h Outdated
Comment thread src/nmodl/nmodlfunc.h Outdated
Comment thread src/nrniv/nmodlrandom.cpp
Comment thread test/coreneuron/test_rng.py Outdated
Comment thread src/oc/nrnran123.cpp Outdated
Comment thread src/nrniv/nmodlrandom.cpp Outdated
@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@ohm314 ohm314 changed the title New NMODL keyword for random variables. NEURON { RANDOM123 ranvar } New NMODL keyword for random variables. NEURON { RANDOM ranvar } Feb 5, 2024
@bbpbuildbot

This comment has been minimized.

@azure-pipelines

Copy link
Copy Markdown

✔️ 8061d28 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@azure-pipelines

Copy link
Copy Markdown

✔️ a8dab46 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

pramodk added a commit to BlueBrain/nmodl that referenced this pull request Feb 8, 2024
…tatement (#1125)

* Support for new RANDOM construct in NMODL, see neuronsimulator/nrn#2627
* Added necessary changes to the parser, visitors, symbol table and code generation
* Added unit tests

Co-authored-by: Pramod S Kumbhar <pramod.s.kumbhar@gmail.com>
@bbpbuildbot

This comment has been minimized.

@azure-pipelines

Copy link
Copy Markdown

✔️ 2197723 -> Azure artifacts URL

@sonarqubecloud

sonarqubecloud Bot commented Feb 8, 2024

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
93 New issues

Measures
0 Security Hotspots
No data about Coverage
2.7% Duplication on New Code

See analysis details on SonarCloud

@azure-pipelines

Copy link
Copy Markdown

✔️ e38cd81 -> Azure artifacts URL

@pramodk pramodk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - I believe multiple people have already looked and agreed current state is good to get this flying! 🚀

@pramodk pramodk merged commit 758d15c into master Feb 8, 2024
@pramodk pramodk deleted the hines/nmodlrandom-simplified branch February 8, 2024 10:57
pramodk added a commit that referenced this pull request Mar 19, 2024
  - after RANDOM construct (#2627) was merged,
    netstim.mod uses Random123 by default
  - before this, netstim was using scoprand by default
    and reference results were from the same
  - now, explicitly use Random123 in the test and update
    reference results
ohm314 pushed a commit to BlueBrain/nmodl that referenced this pull request May 21, 2024
…tatement (#1125)

* Support for new RANDOM construct in NMODL, see neuronsimulator/nrn#2627
* Added necessary changes to the parser, visitors, symbol table and code generation
* Added unit tests

Co-authored-by: Pramod S Kumbhar <pramod.s.kumbhar@gmail.com>
ramcdougal added a commit that referenced this pull request May 31, 2024
* initial partial API

* nrnpy_set_pr_etal should be extern C

* nrn_call_function leaves results on stack

* segments, properties

* including hocdec.h to try to avoid Datum errors on some systems

* sectionlist and allsec hoc_Item getters

* symbol and object introspection

* symbol table getting and interating

* object ref, push

* Renamed files to avoid having two nrnapi.h files.

On some systems (most? but not the one I used for everything else),
some files tried to include the wrong nrnapi.h during the
"make install" phase.

* neuronapi.cpp no longer uses its .h, that is now a header for 3rd party tools

* switched to enum for stack types

* needed typedefs for C to be happy

* test: sections.c

* clang-format

* replaced hoc_Item with new synonym nrn_Item

* netcon example shows no need for dlsym

* support for .so files in addition to .dylib

* removed an excess const

We'll still treat the pointer as const, but the user has no reason to care.

* Add Robert's API tests to CTest (#2365)

* build + run tests

* fixups

* different fixup

* add -rdynamic

* bugfix flagged by ASan

* fix to header file name of api

* hh_sim and sections work with handles
don't have quantitative identity with the old hh_sim results, but very
close.

* rename stack_types_t per @olupton

* replace param_handle with param

* working vclamp

* unified API property functions

* getters use const args

* forgot to commit sections

* vector_capacity now on const obj

* Several improvements

 - C/C++ api header included in implementation
 - signatures made compatible
 - public nrn_Item defined as inheriting from hoc_Item
 - dropped overlapping definitions. types are both fw declarations and
   opaque types

* Make tests link against nrniv_lib, no dlopen

* Address sonar lint reservations

* Make comparisons be part of the test and account for fp innacuraciesa

* Tests to find ref files

* api: Make tests resilient to accumulated errors

* Use in-test ref result, depending on NRN_ENABLE_CORENEURON

* Use free for allocated c strings

* Dont alloc Section Item to heap as its not used.

NOTE: It seems that Sections are never freed. However we are
lacking API to free Sections and Symbols in general.

* Address intel compiler complain

* Free memb_func[].dparam_semantics

* Switch dparam_semantics to unique_ptr (no raw!)

* Memb_list to do its memory management

* Memb_list to be aware of its potential "view" condition and not free

* Back pdata free

* nrn_init to better initialize mpi

* Addressed a number of warnings, inc avoidiing potential double free

* Addressed sonarcloud issues

* fix the merge with master

* Fix test/api/netcon.cpp after merge with master

  - after RANDOM construct (#2627) was merged,
    netstim.mod uses Random123 by default
  - before this, netstim was using scoprand by default
    and reference results were from the same
  - now, explicitly use Random123 in the test and update
    reference results

* fix warning [-Wimplicit-exception-spec-mismatch]

---------

Co-authored-by: Olli Lupton <oliver.lupton@epfl.ch>
Co-authored-by: Fernando Pereira <fernando.pereira@epfl.ch>
Co-authored-by: Pramod Kumbhar <pramod.s.kumbhar@gmail.com>
Co-authored-by: Nicolas Cornu <nicolas.cornu@epfl.ch>
Co-authored-by: Michael Hines <michael.hines@yale.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants