Skip to content
This repository was archived by the owner on Mar 20, 2023. It is now read-only.

Fix assertion in nrn2core_transfer_watch_condition - #623

Merged
pramodk merged 6 commits into
masterfrom
psolve-direct-fix-permute
Aug 30, 2021
Merged

Fix assertion in nrn2core_transfer_watch_condition#623
pramodk merged 6 commits into
masterfrom
psolve-direct-fix-permute

Conversation

@iomaganaris

@iomaganaris iomaganaris commented Aug 26, 2021 •
edited by nrnhines
Loading

Copy link
Copy Markdown
Contributor

Description

assert(pnt->_i_instance == pntindex); was not working properly for permute type different than 0.
Apply fix by @nrnhines mentioned in #619 (comment)

Please include a summary of the change and which issue is fixed or which feature is added.

How to test this?

Follow instructions in #619 (comment)

Test System

  • OS: RedHat
  • Compiler: Intel
  • Version: master branch
  • Backend: CPU

CI_BRANCHES:NEURON_BRANCH=hines/psolve-permute-test,

@iomaganaris iomaganaris mentioned this pull request Aug 26, 2021
Closed
@pramodk

pramodk commented Aug 26, 2021

Copy link
Copy Markdown
Collaborator

@nrnhines was planning to look into the comments in #619. So I will wait for his review / feedback.

@bbpbuildbot

bbpbuildbot commented Aug 26, 2021

Copy link
Copy Markdown
Collaborator

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

Status and direct links:

@nrnhines

nrnhines commented Aug 26, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator

And there is likely at least one more place permutation is a problem. I'm using:

hines@hines-T7500:~/neuron/nrncn/test/coreneuron$ git diff .
diff --git a/test/coreneuron/test_watchrange.py b/test/coreneuron/test_watchrange.py
index ce7bcbede..d4f1d2eab 100644
--- a/test/coreneuron/test_watchrange.py
+++ b/test/coreneuron/test_watchrange.py
@@ -19,6 +19,9 @@ h.dt = 1.0 / h.steps_per_ms
 class Cell:
     def __init__(self, gid):
         self.soma = h.Section(name="soma", cell=self)
+        if gid%2 == 0: # CoreNEURON permutation not the identity if cell topology not homogeneous
+          self.dend = h.Section(name="dend", cell=self)
+          self.dend.connect(self.soma(.5))
         self.gid = gid
         pc.set_gid2node(gid, pc.id())
         self.r = h.Random()

And getting on one of the tests:

hines@hines-T7500:~/neuron/nrncn/test/coreneuron$ python test_watchrange.py
...
cell=0 invalid:(2 6) mode=1
first difference at 3 (0.25, low, r=0.0284617) vs (0.224624,  high, r=0.0284617)
  0 0 (0,  mid, r=0.914602) vs (0,  mid, r=0.914602)
  1 0.125 (0,  high, r=0.914602) vs (0,  high, r=0.914602)
  2 0.25 (0,  high, r=0.914602) vs (0.125,  high, r=0.914602)
  3 0.375 (0.25, low, r=0.0284617) vs (0.224624,  high, r=0.0284617)
...
    assert std[j] == result[j]
AssertionError

@bbpbuildbot

bbpbuildbot commented Aug 26, 2021

Copy link
Copy Markdown
Collaborator

Logfiles from GitLab pipeline #15052 (:no_entry:) have been uploaded here!

Status and direct links:

@nrnhines

nrnhines commented Aug 26, 2021

Copy link
Copy Markdown
Collaborator

one more place permutation is a problem

Presently

static void core2nrn_tqueue_item(TQItem* q, NrnThread& nt) {
...
        case SelfEventType: { 
...
            int tar_index = pnt->_i_instance;

tar_index needs the value from an inverse permutation.

@nrnhines

nrnhines commented Aug 27, 2021

Copy link
Copy Markdown
Collaborator

There is still a highly nonsensical code fragment I need to review in core2nrn_tqueue_item with regard to the self_event_weight_map. It seems to be a copy of a pattern used in the nrn2core direction for SelfEvent but in this context it is bizarrely misapplied. At any rate it would certainly have a permutation problem.

@bbpbuildbot

bbpbuildbot commented Aug 27, 2021

Copy link
Copy Markdown
Collaborator

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

Status and direct links:

@codecov-commenter

codecov-commenter commented Aug 27, 2021

Copy link
Copy Markdown

Codecov Report

Merging #623 (fb9c632) into master (b1c1f68) will decrease coverage by 0.09%.
The diff coverage is 4.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #623      +/-   ##
==========================================
- Coverage   54.37%   54.28%   -0.10%     
==========================================
  Files         102      102              
  Lines        8360     8376      +16     
==========================================
+ Hits         4546     4547       +1     
- Misses       3814     3829      +15
Impacted Files Coverage Δ
coreneuron/io/nrn2core_data_init.cpp 0.64% <0.00%> (-0.02%) ⬇️
coreneuron/io/core2nrn_data_return.cpp 1.30% <6.66%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1c1f68...fb9c632. Read the comment docs.

@nrnhines

nrnhines commented Aug 27, 2021

Copy link
Copy Markdown
Collaborator

At any rate it would certainly have a permutation problem.
Turns out that is not the case as nt.pntprocs is never permuted.

@bbpbuildbot

bbpbuildbot commented Aug 27, 2021

Copy link
Copy Markdown
Collaborator

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

Status and direct links:

@bbpbuildbot

bbpbuildbot commented Aug 27, 2021

Copy link
Copy Markdown
Collaborator

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

Status and direct links:

@nrnhines nrnhines mentioned this pull request Aug 27, 2021
Merged
pramodk
pramodk approved these changes Aug 30, 2021
View reviewed changes

@pramodk pramodk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@pramodk
pramodk merged commit a91f2ec into master Aug 30, 2021
@pramodk
pramodk deleted the psolve-direct-fix-permute branch August 30, 2021 13:32
pramodk pushed a commit to neuronsimulator/nrn that referenced this pull request Aug 30, 2021
@nrnhines
Substantive non-identity permute tests for coreneuron (#1430)
3579f1d 
* Update coreneuron submodule : BlueBrain/CoreNeuron#623
pramodk pushed a commit to neuronsimulator/nrn that referenced this pull request Nov 2, 2022
@iomaganaris
Fix assertion in nrn2core_transfer_watch_condition (BlueBrain/CoreNeu…
a8fa278 
…ron#623)

* Fix assertion in nrn2core_transfer_watch_condition realted to permuation type
* Another place (SelfEvent) where permute was not checked.
* Core2Nrn SelfEvent targets need inverse permutation
* Core2nrn WATCH info needs inverse, not forward, permutation.
* Only one iteration over nt.netcons for SelfEvent core2nrn.

Co-authored-by: Michael Hines <michael.hines@yale.edu>

CoreNEURON Repo SHA: BlueBrain/CoreNeuron@a91f2ec
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@pramodk pramodk pramodk approved these changes
@nrnhines nrnhines nrnhines approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Issue with direct mode on olfactory-bulb-3d model

5 participants

@iomaganaris @pramodk @bbpbuildbot @nrnhines @codecov-commenter