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.

Sign up for GitHub

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

Jump to bottom

ExternalProject SUNDIALS 3.2.1 #1960

Open
wants to merge 123 commits into
base: master
Choose a base branch
from
Open

ExternalProject SUNDIALS 3.2.1 #1960

wants to merge 123 commits into from

Conversation

alexsavulescu
Copy link
Member

@alexsavulescu alexsavulescu commented Aug 20, 2022 •
edited by nrnhines
Loading

  • include work from cvode3 branch

closes #1328

  • Cvode version 3 test data must be visually compared to the legacy data for acceptable similarity.
  • Must use nrn-modeldb-ci to visually compare the differences.
  • test coverage substantially complete
  • merge rxd testdata and update submodule commit

@alexsavulescu alexsavulescu mentioned this pull request Aug 20, 2022
Open
@alexsavulescu alexsavulescu changed the title Slds ExternalProject SUNDIALS 3.2.1 Aug 20, 2022
@alexsavulescu alexsavulescu marked this pull request as draft August 20, 2022 12:22
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@alexsavulescu alexsavulescu linked an issue May 3, 2023 that may be closed by this pull request
Drop legacy SUNDIALS and go upstream #1328
Open
@bbpbuildbot

This comment has been minimized.

Sign in to view
@nrnhines
Copy link
Member

nrnhines commented May 5, 2023

@alkino Just to let you know, I have a fix for ctest -R netpar but need to extend to NRNMPI_DYNAMICLOAD.

@alkino
Copy link
Member

alkino commented May 5, 2023

@alkino Just to let you know, I have a fix for ctest -R netpar but need to extend to NRNMPI_DYNAMICLOAD.

NRNMPI_DYNAMICLOAD should be cleaned to be the same than in coreneuron (double implentation today): #2285, so feel free to rework everything

@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@nrnhines nrnhines requested a review from pramodk October 28, 2024 17:42
@nrnhines
Copy link
Member

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

pramodk
pramodk reviewed Oct 28, 2024
View reviewed changes
Copy link
Member

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

Glad to see this is close to becoming a reality!

I didn’t have time to review the C++ changes, but I think @alkino can help with that.

I have just added two minor comments regarding the build parts.

cmake/modules/FindSUNDIALS.cmake Show resolved Hide resolved
cmake/modules/FindSUNDIALS.cmake
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)
Copy link
Member

Choose a reason for hiding this comment

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

This was previously discussed with Alex, but still remember less-than-ideal experiences with ExternalProject_Add during 2016-17. I am still wondering if there might be better alternatives to using nested cmake configure steps. :)

I'll leave it to @alkino and @1uc to weigh in.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

src/nrniv/nvector_nrnparallel_ld.cpp Show resolved Hide resolved
@nrnhines
Copy link
Member

@pramodk I was looking at the checkbox you added to the top commit
merge rxd testdata and update submodule commit
and was wondering what is meant. The nrn master already has

commit f8e94c53da9d59570e6922acef6715f85776ff2e
Author: adamjhn <adam.newton@yale.edu>
Date:   Thu Oct 24 14:59:12 2024 -0400

    Move reference data from test_rxd.py to test_rxd.json. (#3143)
    
    
    * Added a --save flag to regenerate the data.

and the sundials version that this PR uses is mentioned in nrn/cmake/modules/FindSUNDIALS.cmake

GIT_REPOSITORY https://github.com/neuronsimulator/sundials.git
GIT_TAG 71b3daec18ce66a7c011be9501a31567aa5c09b6 # 3.2.1 hines/hoc_pow

So it seems the checkbox should have an [x]

@azure-pipelines Azure Pipelines
Copy link

✔️ 1c2308c -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines Azure Pipelines
Copy link

✔️ 7286d43 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Sign in to view
@nrnhines nrnhines mentioned this pull request Nov 4, 2024
Open
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Nov 6, 2024

Quality Gate Passed Quality Gate passed

Issues
169 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.8% Duplication on New Code

See analysis details on SonarCloud

@bbpbuildbot

This comment has been minimized.

Sign in to view
@azure-pipelines Azure Pipelines
Copy link

✔️ d38be72 -> Azure artifacts URL

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Dec 9, 2024

Quality Gate Passed Quality Gate passed

Issues
166 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.8% Duplication on New Code

See analysis details on SonarQube Cloud

@azure-pipelines Azure Pipelines
Copy link

✔️ 091c5c0 -> Azure artifacts URL

@bbpbuildbot
Copy link
Collaborator

Logfiles from GitLab pipeline #240172 (:no_entry:) have been uploaded here!

Status and direct links:

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

@nrnhines nrnhines nrnhines left review comments

@pramodk pramodk pramodk left review comments

@alkino alkino alkino left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release v9.0
Development

Successfully merging this pull request may close these issues.

Drop legacy SUNDIALS and go upstream
5 participants
@alexsavulescu @bbpbuildbot @nrnhines @alkino @pramodk