Skip to content

Default node order is cell contiguous (except for thread rootnodes). #3300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 19 commits into from
May 30, 2025

Conversation

nrnhines
Copy link
Member

@nrnhines nrnhines commented Jan 6, 2025

Default cell contiguity on top of #3299 implies that in general, CvMemList.ml.size() == 1 for each cell. The exceptions
are that CvMembList.ml.size() == 2 whenever the extracellular mechanism is present or if a synapse (of the same type) is located both in the cell rootnode and somewhere else on the cell (note that cell rootnodes for a thread are adjacent).
The node ordering of each cell (after the root node) is section depth first.

This ordering fixes most of the performance loss with respect to 8.2. With the intel compiler, performance of the ringtest with
nrniv -python ringtest.py -rparm -nring 32 -npt 1 -compart 1 3 -method 2 -2nd_order_thresh is

version    runtime (s)
8.2        1.4
master     6.7
This PR    2.7

I speculate that the remainder of the performance issue is the use of DataHandle during sim time instead of raw double* and that will be dealt with in subsequent pull requests.

I would like this PR to be reviewed as a change to #3299 but not merged. It can be merged to the master after #3299 is merged into the master.

@nrnhines nrnhines requested a review from JCGoran January 6, 2025 17:55
@nrnhines nrnhines marked this pull request as draft January 6, 2025 17:56
Copy link

✔️ 4f6cba4 -> Azure artifacts URL

Copy link

sonarqubecloud bot commented Jan 8, 2025

Base automatically changed from hines/cvlocal-bug to master May 29, 2025 19:24
Copy link

✔️ 67edba4 -> Azure artifacts URL

Copy link

codecov bot commented May 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.34%. Comparing base (228e30c) to head (63b49b9).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3300      +/-   ##
==========================================
- Coverage   68.34%   68.34%   -0.01%     
==========================================
  Files         682      682              
  Lines      116580   116572       -8     
==========================================
- Hits        79680    79672       -8     
  Misses      36900    36900              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

✔️ 67edba4 -> artifacts URL

Copy link
Contributor

✔️ 4a69c68 -> artifacts URL

Copy link

✔️ 4a69c68 -> Azure artifacts URL

Copy link
Contributor

✔️ 170d40e -> artifacts URL

Copy link
Contributor

✔️ b62acb7 -> artifacts URL

@nrnhines nrnhines marked this pull request as ready for review May 30, 2025 00:17
Copy link

✔️ b62acb7 -> Azure artifacts URL

Copy link
Contributor

✔️ 595a3b4 -> artifacts URL

Copy link

✔️ 3d8a428 -> Azure artifacts URL

@nrnhines nrnhines enabled auto-merge (squash) May 30, 2025 14:10
Copy link

Copy link

✔️ 63b49b9 -> Azure artifacts URL

Copy link
Contributor

✔️ 63b49b9 -> artifacts URL

@nrnhines nrnhines merged commit 07c7565 into master May 30, 2025
43 checks passed
@nrnhines nrnhines deleted the hines/cell-contiguous branch May 30, 2025 16:25
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.

2 participants