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

Drop legacy SUNDIALS and go upstream #1328

Open
alexsavulescu opened this issue Jun 11, 2021 · 7 comments · May be fixed by #1960
Open

Drop legacy SUNDIALS and go upstream #1328

alexsavulescu opened this issue Jun 11, 2021 · 7 comments · May be fixed by #1960
Assignees
Labels
dev Developer Tickets refactoring

Comments

@alexsavulescu
Copy link
Member

Today we have a very old implementation copy in neuron.
Sundials is now on Github : https://github.com/LLNL/sundials

  • Supports CMake integration
  • Binary differences to be expected. However that is already the case for variable timestep between two different neuron versions.
@nrnhines
Copy link
Member

https://github.com/neuronsimulator/nrn/tree/cvode3 will help with this along with https://github.com/neuronsimulator/nrn/tree/idainit
Sadly it is prior to the c++ transition but I think almost all the changes were in c++ anyway. It's been languishing because of two major issues. 1) The desire (in the idainit branch) to abandon my home-build initialization algorithm and replace by IDA's internal initialization method. That requires an extended discussion. 2) Make the API transition to the latest Sundials NVector implementation for MPI and Threads. I'd like to remove the files in nrn/src/nrniv called:

nvector_nrnparallel_ld.cpp
nvector_nrnserial_ld.cpp
nvector_nrnthread.cpp
nvector_nrnthread_ld.cpp

and their corresponding h files.

@alexsavulescu
Copy link
Member Author

If we cannot ensure binary compatibility, this issue will belong to a new milestone 9.0

@ramcdougal
Copy link
Member

Unfortunately, CVode in general has not given binary identical results from version to version. (But it's not like an uninitialized memory issue because the results for a given version of NEURON seem to always be consistent.)

That means a few things to me:

(1) if using the same CVode could produce different results, then switching to a different CVode and getting binary different results isn't inherently any worse.

(2) something, somewhere is subtly wrong with what NEURON is doing with CVode in that it depends on some other aspect of the implementation.

My impression has been that each version of NEURON produces one of two possible CVode outputs, but I haven't explicitly tested this hypothesis.

@alexsavulescu alexsavulescu added this to the Release v8.1 milestone Jun 11, 2021
@alexsavulescu
Copy link
Member Author

We'd also need more sundials testing, current coverage for src/sundials is 12.6%
https://codecov.io/gh/neuronsimulator/nrn/tree/master/src/sundials

@nrnhines
Copy link
Member

One difference (between windows and linux) has been traced to pow and exp. See #1324. But that does not explain the occasional difference on the same machine when different nrn versions are compared. One of the reasons for the https://github.com/neuronsimulator/nrn/tree/cvode3 was the hope that the latter issue would go away. The nrntest repostory tests already have been updated to accept the original and Sundial version 3 results.

@nrnhines
Copy link
Member

My impression has been that each version of NEURON produces one of two possible CVode outputs

Thanks. I did not realize that. (Interesting but doesn't immediately elicit a hypothesis for me:) I've only experienced this strange behavior with some modeldb models.

@alexsavulescu
Copy link
Member Author

alexsavulescu commented Aug 20, 2022

Experimented with ExternalProject_Add in #1960. Got most of the work from cvode3 branch (maybe I missed something, I took everything by hand). Works locally on my M1 with make (now it also works with ninja) and I can do make test but there are some failing tests I didn't get the chance to look at.

@alexsavulescu alexsavulescu linked a pull request May 3, 2023 that will close this issue
4 tasks
@pramodk pramodk removed this from the Release v9.0 milestone Sep 9, 2024
@nrnhines nrnhines linked a pull request Oct 24, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev Developer Tickets refactoring
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants