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

use CVODES error string instead of numeric flag in error messages #2620

Merged
merged 9 commits into from
Dec 6, 2021

Conversation

yizhang-yiz
Copy link
Contributor

@yizhang-yiz yizhang-yiz commented Nov 23, 2021

This PR addresses #1039 by replacing flag with strings from CVODES.

Summary

The replacement happens inside the checking functions. With added cvodes_check we call the library functions that map flag to error string constant.

Tests

All CVODES unit tests with error throw checks.

Side Effects

na

Release notes

More informative error messages for ODE solvers.

Checklist

  • Math issue Add better error handling for CVODES #1039.

  • Copyright holder: Metrum Research Group

    By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
    - Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
    - Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

  • the basic tests are passing

    • unit tests pass (to run, use: ./runTests.py test/unit)
    • header checks pass, (make test-headers)
    • dependencies checks pass, (make test-math-dependencies)
    • docs build, (make doxygen)
    • code passes the built in C++ standards checks (make cpplint)
  • the code is written in idiomatic C++ and changes are documented in the doxygen

  • the new changes are tested

@yizhang-yiz
Copy link
Contributor Author

@syclik do you want to take a look of this one?

@yizhang-yiz yizhang-yiz changed the title use CVODES & KINSOL error string instead of numeric flag in error messages use CVODES error string instead of numeric flag in error messages Dec 1, 2021
@yizhang-yiz
Copy link
Contributor Author

@wds15 would you like to review it? thx

@stan-buildbot
Copy link
Contributor


Name Old Result New Result Ratio Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan 3.58 3.55 1.01 0.86% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan 0.02 0.02 1.0 -0.38% slower
eight_schools/eight_schools.stan 0.09 0.09 1.01 0.54% faster
gp_regr/gp_regr.stan 0.15 0.14 1.04 3.65% faster
irt_2pl/irt_2pl.stan 5.74 5.77 0.99 -0.55% slower
performance.compilation 92.78 90.76 1.02 2.18% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan 8.03 8.11 0.99 -0.96% slower
pkpd/one_comp_mm_elim_abs.stan 32.76 29.99 1.09 8.45% faster
sir/sir.stan 120.09 122.12 0.98 -1.69% slower
gp_regr/gen_gp_data.stan 0.04 0.03 1.04 3.6% faster
low_dim_gauss_mix/low_dim_gauss_mix.stan 3.04 2.98 1.02 1.89% faster
pkpd/sim_one_comp_mm_elim_abs.stan 0.41 0.38 1.08 7.64% faster
arK/arK.stan 2.07 2.08 1.0 -0.35% slower
arma/arma.stan 0.23 0.23 1.01 0.74% faster
garch/garch.stan 0.58 0.57 1.01 1.37% faster
Mean result: 1.01923991221

Jenkins Console Log
Blue Ocean
Commit hash: ee568be


Machine information ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

@yizhang-yiz
Copy link
Contributor Author

This is ready for review. @SteveBronder @rok-cesnovar any of you have time for this? It's a straightforward impl.

Copy link
Member

@rok-cesnovar rok-cesnovar left a comment

Choose a reason for hiding this comment

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

Overall I think this is a great improvement of the current state. Thanks @yizhang-yiz!

I commented in a few spots that things could be slightly improved.

Copy link
Member

@rok-cesnovar rok-cesnovar left a comment

Choose a reason for hiding this comment

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

Looks great. Will probably take a day or a few to get Jenkins tests to pass again. Merge whenever the green light is back.

@yizhang-yiz
Copy link
Contributor Author

thanks a lot @rok-cesnovar

@stan-buildbot
Copy link
Contributor


Name Old Result New Result Ratio Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan 3.59 3.54 1.01 1.4% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan 0.02 0.02 0.97 -3.49% slower
eight_schools/eight_schools.stan 0.08 0.09 0.99 -1.22% slower
gp_regr/gp_regr.stan 0.14 0.14 1.01 0.8% faster
irt_2pl/irt_2pl.stan 5.7 5.82 0.98 -2.05% slower
performance.compilation 92.97 90.96 1.02 2.16% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan 8.03 8.04 1.0 -0.16% slower
pkpd/one_comp_mm_elim_abs.stan 32.11 31.01 1.04 3.44% faster
sir/sir.stan 121.28 117.04 1.04 3.5% faster
gp_regr/gen_gp_data.stan 0.03 0.03 0.97 -2.74% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan 3.01 3.16 0.95 -5.15% slower
pkpd/sim_one_comp_mm_elim_abs.stan 0.39 0.38 1.02 2.3% faster
arK/arK.stan 2.06 2.05 1.0 0.49% faster
arma/arma.stan 0.23 0.23 1.0 0.35% faster
garch/garch.stan 0.58 0.58 1.01 0.76% faster
Mean result: 1.0008438709

Jenkins Console Log
Blue Ocean
Commit hash: 74c37fd


Machine information ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

@rok-cesnovar rok-cesnovar merged commit 3a080d7 into develop Dec 6, 2021
@rok-cesnovar rok-cesnovar deleted the cvodes_err_msg_issue_1039 branch December 6, 2021 05:32
@yizhang-yiz yizhang-yiz mentioned this pull request Jan 7, 2022
5 tasks
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.

4 participants