ExternalProject SUNDIALS 3.2.1 #1960
Conversation
* include work from cvode3 branch
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@alkino Just to let you know, I have a fix for |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
-DNRN_ENABLE_ARCH_INDEP_EXP_POW=ON -DCMAKE_C_FLAGS="-ffp-model=strict" -DCMAKE_CXX_FLAGS="-ffp-model=strict"
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1960 +/- ##
==========================================
+ Coverage 67.11% 70.58% +3.47%
==========================================
Files 571 547 -24
Lines 111318 103052 -8266
==========================================
- Hits 74714 72743 -1971
+ Misses 36604 30309 -6295 ☔ View full report in Codecov by Sentry. |
This comment has been minimized.
This comment has been minimized.
needed for 4 ubuntu CI. Not sure about the underlying cause.
|
✔️ b398627 -> Azure artifacts URL |
This comment has been minimized.
This comment has been minimized.
|
@pramodk Feel free to review as briefly as you wish. The most important remaining aspects are for me to check the three boxes in the beginning comment. |
| GIT_REPOSITORY https://github.com/neuronsimulator/sundials.git | ||
| # GIT_TAG e2f29c34f324829302037a1492db480be8828084 6.2.0 -> CVodeMem no longer "visible" GIT_TAG | ||
| # c09e732080a214694b209032ec627c93fed45340 4 | ||
| #GIT_TAG 811234254d37652954daff0ccdb7af9813736846 # 3.2.1 | ||
| GIT_TAG 71b3daec18ce66a7c011be9501a31567aa5c09b6 # 3.2.1 hines/hoc_pow | ||
| # GIT_TAG 7ed895fd102226dbc52225e6b2c6e26ef1cafa0e #2.7.0 | ||
| # cmake-format: on |
There was a problem hiding this comment.
@nrnhines : is this LLNL/sundials@main...neuronsimulator:sundials:hines/hoc_pow only change that we need to have? Indeed it's look very trivial.
There was a problem hiding this comment.
That is correct for Sundials version 3. We'll eventually see if changes need to be more substantial for later versions. There is some question about where to put the 10 nrn/src/nrniv/nvector_* files. They distinguish between (serial, thread, mpi) and (double, longdouble) and I don't think the changes are relevant to sundials as they have details that depend on how NEURON uses threads and mpi and NRN_ENABLE_MPI_DYNAMIC. At the moment, when
NRN_ENABLE_MPI_DYNAMIC=ON, then nm libnrniv.so | grep MPI shows nothing that points into libmpi. Instead, many things point into mpi specific interface libraries such as libnrnmpi_mpich.so and libnrnmpi_ompi.so . I think the worst case, ultimately, is that sundials gets built for each ```-DNRN_MPI_DYNAMIC="mpi;versions" and then all of sundials gets linked into each of the various libnrnmpi_.so
| BUILD_BYPRODUCTS | ||
| <INSTALL_DIR>/lib/libsundials_ida.a <INSTALL_DIR>/lib/libsundials_cvode.a | ||
| <INSTALL_DIR>/lib/libsundials_nvecserial.a <INSTALL_DIR>/lib/libsundials_nvecpthreads.a | ||
| <INSTALL_DIR>/lib/libsundials_nvecparallel.a) |
There was a problem hiding this comment.
There was a problem hiding this comment.
My experience with ExternalProject_Add for Sundials hasn't been too bad. At least any change to a build at the nrn level propagates to re-compile and build the sundials *.a files (that link into libnrniv.so). It could be better in terms of having a build tree as a subtree in the nrn build tree. (I often use mulitple nrn/build.../ with different cmake option choices and it would be nice if the latest nrn build did not cause the previous sundial build to be overwritten. I would also prefer to avoid git status not showing the untracked folder external/sundials At the moment the sundials repository seems to begin at nrn/external/sundials/src/sundials-external, the build takes place in nrn/external/sundials/src/sundials-external-build, and the install takes place in nrn/external/sundials/(include, lib). I suppose the way we have been handling InterViews and, more recently, fmt, might provide an alternative strategy.
There was a problem hiding this comment.
I'm all in for ExternalProject_Add 🚀 given that we can do custom build/configure commands, it will be definitely useful for upgrading to newer sundials.
| #include "nvector_nrnparallel_ld.h" | ||
| #include "sundialsmath.h" | ||
| #include "sundialstypes.h" | ||
| #define MPI_Comm int // or perhaps get rid of them all |
There was a problem hiding this comment.
nitpick - just curious about this
There was a problem hiding this comment.
I am seriously considering removing all references to MPI_Comm in nrn/src/nrniv nvector_parallel.* and nvector_nrnparallel_ld.* . These are basically copies of sundials files from version 2 and 3 but modified to allow
construction from within nrn/src/nrncvode/cvodeobj.cpp and also work without directly calling anything from within mpi.h. That is, they are valid for -DNRN_ENABLE_MPI_DYNAMIC= ON or OFF and don't involve any mpi.h structures (such as MPI_Comm) or functions. They only call things available from nrn/src/nrnmpi/nrnmpi.h. I left in the MPI_Comm tokens just as indicators of what might have to be changed in future movment toward Sundials version 7. At the moment, all of SUNDIALS used by neuron is inside libnrniv.so
|
@pramodk I was looking at the checkbox you added to the top commit and the sundials version that this PR uses is mentioned in nrn/cmake/modules/FindSUNDIALS.cmake So it seems the checkbox should have an [x] |
| -DBUILD_SHARED_LIBS=OFF | ||
| -DMPI_C_COMPILER=${MPI_C_COMPILER} | ||
| -DCMAKE_INSTALL_LIBDIR=${SUNDIALS_LIB_DIR} | ||
| BUILD_BYPRODUCTS |
There was a problem hiding this comment.
FYI: this is needed for Ninja
|
✔️ 1c2308c -> Azure artifacts URL |
This comment has been minimized.
This comment has been minimized.
|
|
✔️ 7286d43 -> Azure artifacts URL |
closes #1328