Skip to content

corenrn permute for reports - #3587

Merged
cattabiani merged 3 commits into
masterfrom
katta/cell_permute_corenrn
Sep 5, 2025
Merged

corenrn permute for reports#3587
cattabiani merged 3 commits into
masterfrom
katta/cell_permute_corenrn

Conversation

@cattabiani

@cattabiani cattabiani commented Sep 2, 2025

Copy link
Copy Markdown
Member

Context

Until now, corenrn reports were not really addressing corenrn permutations. They were not tested too since cell_permute is hardcoded to 0 in neurodamus.

Scope

This small pr fixes this to the root (where possible).

  • compartment sets report: in this case the report passes manually the node indexes. At the loading moment NrnThread is empty and I cannot do the node conversion on the spot (@nrnhines if you know of a better way I would actually prefer to permute immediately, when I load this file. I mean in report_configuration_parser.cpp. Let me know if you want a zoom call). For this reason the permutation is done in get_compartment_set_vars_to_report
  • all the other reports: use cell_mapping. We apply the permutation as soon as we load the indexes in memory thus nobody can uses stale values. This is done largely in phase3.cpp.

Tests

This was already successfully tested manually in neurodamus for all the reports (lfp included).

  • run tests with cell_permute 1 once the option can be passed to neurodamus

Note

This pr does not block nor is blocked by the neurodamus side. Current neurodamus has hardcoded cell_permute = 0 and this pr should not change anything if cell_permute is 0

@cattabiani cattabiani self-assigned this Sep 2, 2025
@cattabiani cattabiani added the bug label Sep 2, 2025
@github-actions

github-actions Bot commented Sep 2, 2025

Copy link
Copy Markdown
Contributor

✔️ fb544b3 -> artifacts URL

@azure-pipelines

Copy link
Copy Markdown

✔️ fb544b3 -> Azure artifacts URL

@codecov

codecov Bot commented Sep 2, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.49%. Comparing base (71cb817) to head (4509cae).
⚠️ Report is 41 commits behind head on master.

Files with missing lines Patch % Lines
src/coreneuron/io/nrn_filehandler.hpp 50.00% 1 Missing ⚠️
src/coreneuron/io/phase3.cpp 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3587      +/-   ##
==========================================
- Coverage   68.50%   68.49%   -0.01%     
==========================================
  Files         686      686              
  Lines      116754   116758       +4     
==========================================
- Hits        79978    79971       -7     
- Misses      36776    36787      +11     

☔ 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.

@sonarqubecloud

sonarqubecloud Bot commented Sep 5, 2025

Copy link
Copy Markdown

Comment thread src/coreneuron/io/reports/report_handler.cpp
@github-actions

github-actions Bot commented Sep 5, 2025

Copy link
Copy Markdown
Contributor

✔️ 4509cae -> artifacts URL

@azure-pipelines

Copy link
Copy Markdown

✔️ 4509cae -> Azure artifacts URL

cattabiani added a commit to openbraininstitute/neurodamus that referenced this pull request Sep 5, 2025
… (gpu)}`.

Neurodamus at the moment is not leveraging this. We should allow users to set this.

**Note:** this is different from `cell-permute` for neuron (9+ only).
I may enable that in the future. In that case this will become useless legacy code.
At any rate, neuron 9 is new and not fully used everywhere. Proceeding step by step,
enabling `cell-permute` in coreneuron is the next logical step

* add option in CLI and in the python bindings

* test that it is written in `sim.conf`
* test that `integration-e2e/test_reports.py` is unaffected
* add test in `test_reports.py`. I am using [this](neuronsimulator/nrn#3587)
locally. It will fail in the CI until that one is merged

@JCGoran JCGoran 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

@cattabiani
cattabiani merged commit 28d3612 into master Sep 5, 2025
43 checks passed
@cattabiani
cattabiani deleted the katta/cell_permute_corenrn branch September 5, 2025 13:21
@nrnhines

nrnhines commented Sep 5, 2025

Copy link
Copy Markdown
Member

would actually prefer to permute immediately, when I load this file.

That would only be possible if you loaded the file after the permutation was calculated. I suppose that means create_report_configurations would have to be called after (or inside after permutations are calculated) nrn_init_and_load_data. Looking at the use of interleave_permute_type I see that the permutations are calculated in io/phase2.cpp :: Phase2::populate called from nrn_setup.cpp

@cattabiani

Copy link
Copy Markdown
Member Author

@nrnhines ok. I will revisit if this starts to be a problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants