Skip to content
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

Add test using mcna.mod #2308

Merged
merged 17 commits into from
Jun 23, 2023
Merged

Add test using mcna.mod #2308

merged 17 commits into from
Jun 23, 2023

Conversation

olupton
Copy link
Collaborator

@olupton olupton commented Mar 29, 2023

Originally motivated by BlueBrain/mod2c#92.

mcna.mod, which was already included in NEURON, showed the same issue when compiled for CoreNEURON.
This mechanism was not used in any NEURON tests, but it was used in a test from nrntest that is deleted from there in neuronsimulator/nrntest#30 and reimplemented in Python in this PR.

Unfortunately this MOD file does not compile when translated for CoreNEURON with NMODL (the original issue was using mod2c, but that is now deprecated). Fixing CoreNEURON + NMODL execution of this test is the subject of a follow-up ticket: #2397. Note that this use of PROTECT with GLOBAL variables is documented: https://nrn.readthedocs.io/en/latest/hoc/modelspec/programmatic/mechanisms/nmodl2.html#protect.

Also make some other improvements to tests and test helpers, generally in the direction of ensuring that tests do not leak changes to global NEURON state that cause other tests to fail:

  • The coreneuron class/module (from neuron import coreneuron) can now be used as a context manager:
from neuron import coreneuron
with coreneuron(gpu=True, cell_permute=2):
  assert coreneuron.gpu
  assert coreneuron.cell_permute == 2
# old values are restored here

Finally, reorganise the pytest-based tests slightly:

  • the old pynrn test group is renamed to pytest_coreneuron: MOD files under test/pytest_coreneuron are built for CoreNEURON and test_*.py files are executed using pytest
  • a new pytest group is added: MOD files under test/pytest are only built for NEURON, but pytest is still used to execute test_*.py files

and simplify the fast_imem test a bit.

TODO:

@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Merging #2308 (7e576ce) into master (7197b37) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2308      +/-   ##
==========================================
+ Coverage   60.18%   60.20%   +0.02%     
==========================================
  Files         627      627              
  Lines      121007   121062      +55     
==========================================
+ Hits        72827    72887      +60     
+ Misses      48180    48175       -5     
Impacted Files Coverage Δ
share/lib/python/neuron/coreneuron.py 82.19% <100.00%> (+3.15%) ⬆️
share/lib/python/neuron/rxd/nodelist.py 66.66% <100.00%> (+9.21%) ⬆️
share/lib/python/neuron/tests/utils/__init__.py 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@azure-pipelines
Copy link

✔️ 476b45b -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@olupton olupton force-pushed the olupton/multiple-sparse-statements branch from 476b45b to 49330f1 Compare June 19, 2023 12:24
@azure-pipelines
Copy link

✔️ 49330f1 -> Azure artifacts URL

test/external/CMakeLists.txt Outdated Show resolved Hide resolved
@azure-pipelines
Copy link

✔️ 680e782 -> Azure artifacts URL

Copy link
Member

@alexsavulescu alexsavulescu left a comment

Choose a reason for hiding this comment

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

LGTM to merge once TODOs done

@bbpbuildbot

This comment has been minimized.

@olupton olupton force-pushed the olupton/multiple-sparse-statements branch from df8fab2 to 0b0dd6e Compare June 22, 2023 15:10
@olupton olupton force-pushed the olupton/multiple-sparse-statements branch from 0b0dd6e to 78499a4 Compare June 22, 2023 15:15
@azure-pipelines
Copy link

✔️ 200d578 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@azure-pipelines
Copy link

✔️ c4da059 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

olupton added a commit to neuronsimulator/nrntest that referenced this pull request Jun 23, 2023
@azure-pipelines
Copy link

✔️ 7e576ce -> Azure artifacts URL

@olupton olupton merged commit e029076 into master Jun 23, 2023
@olupton olupton deleted the olupton/multiple-sparse-statements branch June 23, 2023 12:42
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.

3 participants