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

Add unit tests for the Hines solver. #841

Merged
merged 10 commits into from
Jul 12, 2022
Merged

Conversation

olupton
Copy link
Contributor

@olupton olupton commented Jul 11, 2022

Description
Add some tests of the various CPU/OpenACC/OpenMP/CUDA implementations of the Hines matrix solver.

These use toy input data that is generated on the fly. Tests cover scenarios such as:

  • many small cells
  • few large cells

both with hardcoded constant values for the matrix elements and pseudorandom number generation.

The results (d and rhs) from all available solver implementations are compared to one another in each scenario.

See also #826, as some of these tests do not seem to pass at -O0 using NVHPC 22.5. See: #841 (comment).

Also update NMODL to the latest master, which now requires Python 3.7+.

How to test this?
Build CoreNEURON with tests enabled (and Boost), run the test-solver test. Or just execute bin/test-solver.

Test System

  • OS: BB5
  • Compiler: NVHPC 22.3/22.5
  • Version: this feature branch
  • Backend: GPU

Use certain branches in CI pipelines.

CI_BRANCHES:NEURON_BRANCH=master,NMODL_BRANCH=master,SPACK_BRANCH=develop

See also #826, as some of these tests do not seem to pass at -O0 using
NVHPC 22.5.
@bbpbuildbot

This comment was marked as outdated.

@bbpbuildbot

This comment was marked as outdated.

@bbpbuildbot

This comment was marked as outdated.

@bbpbuildbot

This comment was marked as outdated.

@codecov-commenter
Copy link

Codecov Report

Merging #841 (a5e2497) into master (d6ba964) will increase coverage by 0.79%.
The diff coverage is 90.49%.

@@            Coverage Diff             @@
##           master     #841      +/-   ##
==========================================
+ Coverage   57.56%   58.36%   +0.79%     
==========================================
  Files         101      102       +1     
  Lines        9189     9410     +221     
==========================================
+ Hits         5290     5492     +202     
- Misses       3899     3918      +19     
Impacted Files Coverage Δ
coreneuron/mechanism/eion.cpp 55.90% <ø> (ø)
coreneuron/sim/solve_core.cpp 100.00% <ø> (ø)
tests/unit/solver/test_solver.cpp 90.49% <90.49%> (ø)
coreneuron/gpu/nrn_acc_manager.cpp 42.50% <0.00%> (+2.50%) ⬆️

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 d6ba964...a5e2497. Read the comment docs.

@olupton olupton added the tests label Jul 12, 2022
@olupton olupton marked this pull request as ready for review July 12, 2022 07:07
@olupton olupton requested a review from nrnhines July 12, 2022 07:27
@olupton
Copy link
Contributor Author

olupton commented Jul 12, 2022

cc: @nrnhines in case you have any particular suggestions about toy data characteristics/topologies that would provide useful extra coverage.

So far the number/size of cells varies, as do the a/b/d/rhs values, but each cell is just a binary tree (possibly a bit unbalanced, depending on the total number of segments per cell that is chosen).

@olupton olupton mentioned this pull request Jul 12, 2022
@olupton
Copy link
Contributor Author

olupton commented Jul 12, 2022

With the additional fixes in #842, I can build and run the new tests with NVHPC 22.5 and -O0 -g plus -tp haswell for nvc/nvc++ and -lineinfo for nvcc.

The CellPermute2_GPU configuration does not match the reference (CellPermute0_CPU) in the SingleCellAndThread, UnbalancedCellSingleThread, LargeCellSingleThread and LargeCellSingleThreadRandom tests, but the ones with smaller cells do pass. It appears that there is some data race issue with parallelisation across a single cell.

At -O1, the issue disappears and the tests added in this PR all pass with NVHPC 22.5.

Copy link
Contributor

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

@olupton olupton merged commit 9523270 into master Jul 12, 2022
@olupton olupton deleted the olupton/solver-unit-tests branch July 12, 2022 12:20
@olupton olupton mentioned this pull request Aug 9, 2022
pramodk pushed a commit to neuronsimulator/nrn that referenced this pull request Nov 2, 2022
* As well as improving test coverage, these aim to
  demonstrate problems with nvc++ 22.5 at -O0 (BlueBrain/CoreNeuron#826).
* OpenMP target offload support for the
  `cell_permute=0` solver implementation.
* Unrelated: update NMODL submodule, which now
  requires Python 3.7+

CoreNEURON Repo SHA: BlueBrain/CoreNeuron@9523270
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants