BinQ and spike compression fixes #701
Conversation
| d.inter_thread_events_.clear(); | ||
| d.tqe_->nshift_ = -1; | ||
| d.tqe_->shift_bin(nrn_threads->_t); | ||
| d.tqe_->shift_bin(nrn_threads->_t - 0.5 * nrn_threads->_dt); |
There was a problem hiding this comment.
There is a similar statement in the following function I did not change. A specific test would have to be written to see if there could be an issue with file mode demonstrating the need for an equivalent change.
There was a problem hiding this comment.
The corresponding change was made in NetCvode::init_events. In retrospect, it is likely that BinQ is not supported for --restore since there is no mention of shift_bin in the nrn_checkpoint files. (Analogous to shift_bin in nrn/src/nrniv/bbsavestate.cpp)
| ps->send(spikein[i].spiketime, net_cvode_instance, nt); | ||
| } | ||
| } | ||
| nrn_multithread_job(interthread_enqueue); |
There was a problem hiding this comment.
I'm not sure this change is needed. But it can't hurt.
|
Logfiles from GitLab pipeline #27163 (:white_check_mark:) have been uploaded here! Status and direct links:
|
|
Logfiles from GitLab pipeline #27361 (:no_entry:) have been uploaded here! Status and direct links:
|
|
Logfiles from GitLab pipeline #27467 (:no_entry:) have been uploaded here! Status and direct links:
|
Release random123 instance when multisend setup no longer needs it. Psolve restores a few more arg default values.
|
Logfiles from GitLab pipeline #27994 (:white_check_mark:) have been uploaded here! Status and direct links:
|
|
Logfiles from GitLab pipeline #29270 (:no_entry:) have been uploaded here! Status and direct links:
|
olupton
left a comment
olupton
left a comment
There was a problem hiding this comment.
First pass through this changeset on its own looks good, I'll move on to the NEURON side.
alexsavulescu
left a comment
alexsavulescu
left a comment
There was a problem hiding this comment.
LGTM. Maybe merge Olli's change from #720 before.
|
Please retest |
Eliminating the warning when Random123 global index does not change, increases the chance of hiding a bug.
Codecov Report
@@ Coverage Diff @@
## master #701 +/- ##
==========================================
+ Coverage 56.14% 56.22% +0.08%
==========================================
Files 107 107
Lines 8947 8966 +19
==========================================
+ Hits 5023 5041 +18
- Misses 3924 3925 +1
Continue to review full report at Codecov.
|
Indeed |
|
Logfiles from GitLab pipeline #29746 (:no_entry:) have been uploaded here! Status and direct links:
|
|
Was thinking about something along the lines of: neuronsimulator/nrn@e686c1d LE: I have removed the commit above and will instead rely on the increase of processors via #723 |
|
Logfiles from GitLab pipeline #30040 (:no_entry:) have been uploaded here! Status and direct links:
|
|
Logfiles from GitLab pipeline #30108 (:no_entry:) have been uploaded here! Status and direct links:
|
|
Now we get |
@alexsavulescu : In case of GPU build, coreneuron is a static library and hence we have to use |
* if PARAMETER variable of RANGE type is updated in VERBATIM block then it's write_count could be 0 even though it's updated in VERBATIM block - such variable can not be declared as `const` instance variable - one such example is https://github.com/nrnhines/tqperf/blob/master/mod/invlfire.mod * Codegen helper visitor now keep track of all symbols that are used in all verbatim blocks and the codegen visitor check this list before deciding if variable can be const or not. - note that variable could be read-only in verbatim block but currently we don't have robut C code analysis capability and it's not worth (yet) * this issue was encountered in BlueBrain/CoreNeuron#701
|
Logfiles from GitLab pipeline #30371 (:no_entry:) have been uploaded here! Status and direct links:
|
* if PARAMETER variable of RANGE type is updated in VERBATIM block then it's write_count could be 0 even though it's updated in VERBATIM block - such variable can not be declared as `const` instance variable - one such example is https://github.com/nrnhines/tqperf/blob/master/mod/invlfire.mod * Codegen helper visitor now keep track of all symbols that are used in all verbatim blocks and the codegen visitor check this list before deciding if variable can be const or not. - note that variable could be read-only in verbatim block but currently we don't have robut C code analysis capability and it's not worth (yet) * this issue was encountered in BlueBrain/CoreNeuron#701 * test update: keep ast node, do not return codegen_c_visitor instance with local AST
/gpfs/bbp.cscs.ch/ssd/gitlab_map_jobs/bbpcihpcproj12/P30438/J115285/_/spack-build/spack-stage-neuron-develop-fwu47zj5ksgprg63usqqvmnfvuvjnlmc/spack-build-fwu47zj/share/nrn/nrnmain.cpp:
622ptxas warning : Conflicting options --device-debug and --generate-line-info specified, ignoring --generate-line-info option
623ptxas warning : Conflicting options --device-debug and --generate-line-info specified, ignoring --generate-line-info option
624Successfully created x86_64/special
625++ find . -type f -name special -print -quit
626+ special_exe=./x86_64/special
627+ mpiexec -n 16 ./x86_64/special -mpi -python test1.py
628MPT ERROR: Not enough slots from job scheduler for requested ranks
629 (HPE HMPT 2.25 10/22/21 03:19:55)I thought this was fixed. @alexsavulescu : was this addressed? i.e. number of cores already increased? (I am not too familiar with the setup...) |
|
My intention for a substantive gpu test is to allow use of a POINT_PROCESS version of |
|
GPFS finally reasonably working: |
* Extending the tqperf repository (test1.py) to the cases of binq and spike compression exposed a half dozen or so bugs in the categories of BinQ initialization, incomplete BinQ queue transfer, and failure to enqueue the interthread event buffers in a timely manner. * The tqperf test now does a SHA1 hash comparison of all spike input and output times of all the artificial cells. This test is run from the latest http://github.com/nrnhines/tqperf.git. See instructions in https://github.com/neuronsimulator/tqperf/blob/master/README.md * This PR depends on the BlueBrain/CoreNeuron#701 * Interthread enqueuing must occur after spike exchange. This is needed for binq + compressed spike exchange + threads. * nrn binq must be initialized before core2nrn queue transfer. * nrn2core queue transfer must also iterate over BinQ. * Refactoring makes nrn2core queue transfer more understandable. * neuron.coreneuron: if --multisend then also --ms-subintervals and --ms-phases * tqperf is added as a ci test * Updated to most recent coreneuron master as submodule
* Bug fixes with regard to option --spkcompress <nspike>
* After compressed spike exchange, do interthread_enqueue.
* nrn binq must be initialized before core2nrn queue transfer.
* nrncore binq must be initialized before nrn2core queue transfer
* interthread buffer must be enqueued at beginning of psolve.
* Initialization of binq consistent everywhere.
* Avoid Random123 globalindex warning if the index has not changed.
Release random123 instance when multisend setup no longer needs it.
Psolve restores a few more arg default values.
* Revert nrnran123.cu
Eliminating the warning when Random123 global index does not change, increases the chance of hiding a bug.
* update nmodl submodule
* ntasks=16 for gpu tests as well
Co-authored-by: Alexandru Săvulescu <alexandru.savulescu@epfl.ch>
Co-authored-by: Olli Lupton <oliver.lupton@epfl.ch>
Co-authored-by: Pramod Kumbhar <pramod.s.kumbhar@gmail.com>
CoreNEURON Repo SHA: BlueBrain/CoreNeuron@318e25a
See neuronsimulator/nrn#1548
Superceded by neuronsimulator/nrn#1556
CI_BRANCHES:NEURON_BRANCH=hines/binq-fix-rebase,