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 use of different Jacobian implementations with 1D solver #1836

Merged
merged 17 commits into from
Feb 1, 2025

Conversation

speth
Copy link
Member

@speth speth commented Jan 2, 2025

Changes proposed in this pull request

  • Generalize PreconditionerBase (now renamed SystemJacobian) to be able represent both preconditioners ($M = I - \gamma J$) and the matrices needed for the pseudo-transient solver used by Sim1D ($M = J - \alpha \Delta t^{-1}$)
  • Refactor the MultiJac to be a derived class of PreconditionerBase and avoid direct inheritance from BandMatrix
  • Add methods to OneDim to allow setting the implementation of SystemJacobian to use
  • Introduce a SystemJacobian implementation that uses Eigen's sparse direct LU factorization
  • Make selection of perturbations used when computing the finite difference Jacobian used by OneDim more flexible. In some cases, different perturbations can reduce the number of residual and Jacobian evaluations required to converge. However, this is not universal so the default values are mostly unchanged.

If applicable, fill in the issue number this pull request is fixing

This is preliminary work toward several planned improvements:

Note: some of the earlier performance gains that I saw turned out to require Jacobian settings that were not as robust as I'd hoped. I think getting reliably better Jacobians and cutting down the Jacobian evaluation time will be best addressed by work towards semi-analytical evaluation, as we've done in the 0D solver.

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

Copy link

codecov bot commented Jan 27, 2025

Codecov Report

Attention: Patch coverage is 72.90323% with 84 lines in your changes missing coverage. Please review.

Project coverage is 74.41%. Comparing base (37db019) to head (afe46f1).
Report is 26 commits behind head on main.

Files with missing lines Patch % Lines
include/cantera/numerics/SystemJacobian.h 55.55% 24 Missing ⚠️
interfaces/cython/cantera/jacobians.pyx 73.46% 13 Missing ⚠️
src/numerics/EigenSparseJacobian.cpp 71.05% 11 Missing ⚠️
src/oneD/MultiJac.cpp 69.23% 8 Missing ⚠️
src/numerics/AdaptivePreconditioner.cpp 50.00% 4 Missing and 2 partials ⚠️
src/oneD/OneDim.cpp 87.75% 5 Missing and 1 partial ⚠️
src/numerics/EigenSparseDirectJacobian.cpp 55.55% 2 Missing and 2 partials ⚠️
include/cantera/numerics/AdaptivePreconditioner.h 0.00% 3 Missing ⚠️
src/oneD/MultiNewton.cpp 83.33% 3 Missing ⚠️
src/oneD/Sim1D.cpp 40.00% 2 Missing and 1 partial ⚠️
... and 2 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1836      +/-   ##
==========================================
+ Coverage   74.39%   74.41%   +0.02%     
==========================================
  Files         382      386       +4     
  Lines       53354    53622     +268     
  Branches     9031     9062      +31     
==========================================
+ Hits        39691    39904     +213     
- Misses      10608    10648      +40     
- Partials     3055     3070      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@speth speth marked this pull request as ready for review January 28, 2025 02:56
Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

@speth ... thanks for taking this on! Overall, this looks great, and I look forward to the follow-up work. As this is part of a larger project, could you create a tracking issue over at Cantera/enhancements?

Beyond, all of my requests are nitpicks and are more about making sure that we start new files fully documented.

include/cantera/numerics/SystemJacobian.h Show resolved Hide resolved
include/cantera/numerics/SystemJacobian.h Show resolved Hide resolved
include/cantera/numerics/EigenSparseJacobian.h Outdated Show resolved Hide resolved
interfaces/cython/cantera/preconditioners.pyx Outdated Show resolved Hide resolved
@ischoegl ischoegl merged commit fdca602 into Cantera:main Feb 1, 2025
48 of 49 checks passed
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.

3 participants