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

Sundials 7.1.1 #17

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

Sundials 7.1.1 #17

wants to merge 37 commits into from

Conversation

langmm
Copy link
Member

@langmm langmm commented Dec 5, 2024

Update to be compatible with latest version of Sundials library (currently 7.1.1).

…ity with Sundials 7.1.1

- Updated call signatures
- Require explicit context provided to create Variables
- Added exports
- Updated cmake method to find Sundials
- Updated README with testing information and additional requirements that were not explicit
- Added conda environment.yml file
- Added Github workflow for testing
- Fixed sizes of ratio vectors in tests
Try making the tests fail for older sundials
Updated README with additional info about managing dependencies via conda
Fixed name of executable in CLI help
Remove additional use of deprecated cvode_direct.h
Define RCONST in RedoxReg_Rate.cpp
Added CLI option for output file
Update Github workflow docs step to use new dev script
…e (was causing test failure when run with coverage)

Updated trDynaPS driver FullTest to cause failure for older sundials
Updated README with minimum tested version of sundials (5.7.0)
Fix forward declaration of CalcData
Export static class variables for Windows compat
Add decimals to test Emap to prevent windows warnings about conversion to double
Try using os compilers for valgrind workflow jobs since conda-forge libraries do not include debug symbols
…(DynaPS test disable for failing with default params)
@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@langmm
Copy link
Member Author

langmm commented Dec 5, 2024

These changes should not modify the calculated results and no internal constants have been modified. In addition to supporting Sundials 7.1.1 changes were also made to support development and improve utility:

  • Added Github actions workflow to run tests
  • Added export macros and export static variables (required for MSVC)
  • Enabled windows builds (tested with MSVC)
  • Added build option WITH_ASAN for enabling address & undefined behavior sanitizers (clang & gcc)
  • Added integration tests for ePhoto executable with each driver
  • Added environment.yml file for managing conda environment
  • Added python script scripts/devtools.py for performing dev tasks
  • Added CLI option for specifying a file containing expression levels (--grn)
  • Added conversion in the CLI from Air_CO2 to CO2_cond and from Radiation_PAR to PPFD_in & TestLi to match how the MATLAB version expected inputs (this should be checked to be as expected by users)
  • Added CLI option for specifying the name of the generated output file
  • Improved help message for driver & rubiscomethod CLI options
  • Modified CLI to allow for the absence of input files
  • Added error raised in CLI if a valid enzyme file is not provided when --c3 is passed
  • Added coverage reporting via (CodeCov)[https://app.codecov.io/gh/cropsinsilico/ePhotosynthesis_C/tree/cvode_7.1.1]
  • Fixed a few memory leaks identified by valgrind
  • Disabled the unused ssPS module
  • Modified configuration of DynaPS driver to enable the connection between the RuACT & EPS modules as is done in the MATLAB version of the code

@langmm langmm marked this pull request as ready for review December 5, 2024 19:58
…on-pointer

Re-enabled tests for RedoxReg module, fixed order of KINSol operations, & added test for RedoxReg output as this module is not currently called via the CLI
Added comment about enabling RedoxReg in trDynaPS driver
…m**2 && CO2 in ppm)

Stripped input file down to just used parameters
Set test input PAR to 2000 umol/s/m**2 (470 W/m**2)
…arams that should be exact reguardless of system/sundials version

Add flag for stopping after first failure that is disabled by default
Add flag for preserving test outputs
@langmm langmm requested a review from yufenghe22 December 12, 2024 19:31
langmm and others added 7 commits December 17, 2024 15:38
…ault to allow for creation of Variables instances without providing a context

Use shared pointer to manage sundials contexts
Declare dummy SUNGetErrMsg inline
Define SUN_COMM_NULL if not defined for older versions of sundials
… before module mode (FindSUNDIALS) and update FindSUNDIALS.cmake to use the same names as the most recent version of Sundials
Make MPI optional to allow build w/ or w/o MPI (Sundials will only link MPI as needed)
Install built Sundials W/ MPI in conda environment
…extension

Add methods for reading files into Variables that can be used by the python extension
Add debug messages for duplicate input parameters and for absent EnzymeAct when useC3 is true
Add missing headers
Fix minor compilation errors & warnings w/ MSVC
Add missing flag for installing the Python extension
…irectory without install

Update README with info on the Python package
Don't re-use CVode solver memory between driver calls
Copy link
Contributor

@yufenghe22 yufenghe22 left a comment

Choose a reason for hiding this comment

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

I merged the cvode_7.1.1 to my working branch SoybeanParameterization. When testing on biocluster, the cmake and compilation were successful. I was also able to repeat my previous results using my own scripts that link to the libEPhotosynthesis.so. A result example is shown below:
image

CMakeLists.txt Show resolved Hide resolved
src/Condition.cpp Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants