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

Enable Fortran interface for 32-bit sunindextype #447

Open
wants to merge 83 commits into
base: develop
Choose a base branch
from

Conversation

balos1
Copy link
Member

@balos1 balos1 commented Mar 26, 2024

This tweaks the SWIG generation, and CMake build system to support the Fortran interfaces for 32-bit or 64-bit sunindextype as suggested in https://github.com/LLNL/sundials-private/issues/60. Most of the files 'changed' are just the moved or new SWIG generated files. Unfortunately this does make the repository larger in terms of files and bytes.

@balos1 balos1 added the Fortran label Mar 26, 2024
@balos1 balos1 added this to the SUNDIALS Next milestone Mar 26, 2024
@balos1 balos1 changed the base branch from main to develop March 26, 2024 22:58
Copy link
Collaborator

@drreynolds drreynolds left a comment

Choose a reason for hiding this comment

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

I still need to test this out, but for now I have only a few comments/questions.

swig/Makefile Show resolved Hide resolved
swig/README.md Show resolved Hide resolved
swig/sunmatrix/fsunmatrix_sparse_mod.i Outdated Show resolved Hide resolved
@balos1
Copy link
Member Author

balos1 commented May 13, 2024

@drreynolds @gardner48 All checks are green, yay! I think this is ready now.

@balos1 balos1 requested a review from drreynolds May 13, 2024 18:00
Copy link
Collaborator

@drreynolds drreynolds left a comment

Choose a reason for hiding this comment

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

One tiny item, otherwise this looks great to me.

Co-authored-by: Daniel R. Reynolds <reynolds@smu.edu>
@balos1 balos1 requested a review from drreynolds May 13, 2024 20:39
drreynolds
drreynolds previously approved these changes May 13, 2024
cmake/SundialsExampleOptions.cmake Show resolved Hide resolved
find_package(KLU CONFIG)
find_package(SuiteSparse_config CONFIG)
Copy link
Member

Choose a reason for hiding this comment

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

While I tried using a newer KLU these other dependencies got picked up by the KLU target, did you have issues with it not finding them? I think I did have to up date the CMAKE_MODULE_PATH though rather than just setting KLU_ROOT or KLU_DIR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, if you set CMAKE_MODULE_PATH it will find them, but if you don't you need these other lines.

Copy link
Contributor

@mmuetzel mmuetzel May 15, 2024

Choose a reason for hiding this comment

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

Could you please show in which case you need to add these lines? It definitely wasn't intended that downstream projects would need to manually include dependencies of a SuiteSparse library.
Is this an issue with the CMake Config files installed by SuiteSparse?

Copy link
Member

@gardner48 gardner48 May 25, 2024

Choose a reason for hiding this comment

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

@mmuetzel The issue seems to be with using -DKLU_DIR=<suite_sparse_install_path>/lib64/cmake/KLU alone to provide the config file location. Using either -DKLU_ROOT=<suite_sparse_install_path> or -DCMAKE_PREFIX_PATH=<suite_sparse_install_path> works without the above additions.

@balos1 are you also setting -D<dependency>_DIR variables for the other packages? Using -DKLU_DIR alone didn't work for me with the above additions. I had add -D<dependency>_DIR variables for each dependency which also makes the config work without the above additions.

This issue is tangential to the topic of this PR so I suggest these changes get broken out into a separate PR.

doc/shared/nvectors/NVector_Description.rst Outdated Show resolved Hide resolved
examples/arkode/F2003_custom/CMakeLists.txt Show resolved Hide resolved
examples/nvector/C_openmp/test_fnvector_openmp_mod.f90 Outdated Show resolved Hide resolved
examples/nvector/test_nvector.f90 Outdated Show resolved Hide resolved
drreynolds
drreynolds previously approved these changes May 24, 2024
@balos1
Copy link
Member Author

balos1 commented May 24, 2024

@gardner48 I think this is ready to merge, if you approve.

CHANGELOG.md Outdated Show resolved Hide resolved
find_package(KLU CONFIG)
find_package(SuiteSparse_config CONFIG)
Copy link
Member

@gardner48 gardner48 May 25, 2024

Choose a reason for hiding this comment

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

@mmuetzel The issue seems to be with using -DKLU_DIR=<suite_sparse_install_path>/lib64/cmake/KLU alone to provide the config file location. Using either -DKLU_ROOT=<suite_sparse_install_path> or -DCMAKE_PREFIX_PATH=<suite_sparse_install_path> works without the above additions.

@balos1 are you also setting -D<dependency>_DIR variables for the other packages? Using -DKLU_DIR alone didn't work for me with the above additions. I had add -D<dependency>_DIR variables for each dependency which also makes the config work without the above additions.

This issue is tangential to the topic of this PR so I suggest these changes get broken out into a separate PR.

doc/shared/RecentChanges.rst Outdated Show resolved Hide resolved
doc/shared/nvectors/NVector_Description.rst Outdated Show resolved Hide resolved
examples/arkode/F2003_parallel/CMakeLists.txt Outdated Show resolved Hide resolved
examples/cvode/F2003_serial/CMakeLists.txt Outdated Show resolved Hide resolved
examples/cvodes/F2003_serial/CMakeLists.txt Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants