Adding adaptive preconditioner functionality to Cantera (draft)#951
Adding adaptive preconditioner functionality to Cantera (draft)#951anthony-walker wants to merge 3 commits intoCantera:mainfrom
Conversation
anthony-walker
force-pushed
the
main
branch
4 times, most recently
from
131a3bf to
0a7d185
Compare
Codecov Report
@@ Coverage Diff @@
## main #951 +/- ##
==========================================
- Coverage 70.67% 70.11% -0.57%
==========================================
Files 362 366 +4
Lines 44554 44915 +361
==========================================
+ Hits 31488 31491 +3
- Misses 13066 13424 +358
Continue to review full report at Codecov.
|
anthony-walker
force-pushed
the
main
branch
2 times, most recently
from
b646c4c to
0d7c06d
Compare
anthony-walker
force-pushed
the
main
branch
5 times, most recently
from
d5a9c79 to
bbb6001
Compare
implementing the adaptive preconditioner inside of cantera.
It adds PreconditionerBase and derives AdaptivePreconditioner from this
base class. PreconditionerBase is intended to be an abstract class so
other types of preconditioners can be extended in the future.
AdaptivePreconditioner is a specific type of preconditioning based on a
tolerance. It was implemented with a double dispatch design pattern which
requires that each reactor type implement a function to work with it.
I have started to implement specific functions for each reactor type.
IdealGasConstPressureReactor - is completed and needs tested still.
It also adds some functions to interact with the integrator more and has
a basic python interface.
Completed simple python interface and plugged into combustor.py with IdealGasConstPressureReactor and it worked w/o convergence error from eigen
Also removed submodule changes to hopefully fix CI
bryanwweber
left a comment
bryanwweber
left a comment
There was a problem hiding this comment.
Hi @anthony-walker Thanks for sharing your work so far! Since this is still in draft mode, I didn't make any comments about the implementation (well, except one I think about SUNDIALS). The comments are some small formatting things to watch out for as you continue working on this. Thanks!
|
|
||
| #include "cantera/numerics/Integrator.h" | ||
| #include "cantera/base/ctexceptions.h" | ||
|
|
There was a problem hiding this comment.
The changes in this file look spurious, can you remove them?
| virtual void setProblemType(int probtype) | ||
| { |
There was a problem hiding this comment.
I'm not sure what changed here, maybe line endings? Can you make sure you're using only "LF" line endings, not "CRLF"?
| { | ||
| class PreconditionerBase | ||
| { | ||
| private: |
There was a problem hiding this comment.
This doesn't seem to be doing anything here.
src/numerics/CVodesIntegrator.cpp
Outdated
| return f->eval_nothrow(t, NV_DATA_S(y), NV_DATA_S(ydot)); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Please don't add any trailing whitespace to lines. You can also configure your editor to remove trailing whitespace.
src/numerics/CVodesIntegrator.cpp
Outdated
| integrator->m_error_message += "\n"; | ||
| } | ||
|
|
||
| #if CT_SUNDIALS_VERSION>=30 //These functions only work with Sundials 3.0 or newer |
There was a problem hiding this comment.
What happens if CT_SUNDIALS_VERSION is less than 3.0? Does the code fail with a useful error message?
There was a problem hiding this comment.
This was updated to pass CI. It throws a CanteraError now. These functions are used within CVodesIntegrator::applyOptions. That is where the error is handled.
|
HI @anthony-walker ... I'm likewise happy to see this moving! To add to @bryanwweber 's preliminary review, I have some other minor formatting comments: |
|
PS: on a separate note, it looks like you're still on your fork's main branch for this PR (as noted elsewhere). I'm not sure that there is a simple fix that preserves the PR # (there may be), but it may make sense to switch to a feature branch on your fork sooner rather than later. |
|
@ischoegl I believe I would have to close the PR to change the branch but I will use a feature branch in the future. |
That's likely true, but it would spare you quite a few headaches whenever there is a need to rebase (as you've seen). Thus far, there isn't much attached to this PR, so re-opening won't lose too much information (i.e. what's above are mainly preliminary comments). Losing the commit record was unfortunate btw ... Choice is yours of course! |
|
Closing pull request to re-open from a feature branch. |
|
Thank you, @anthony-walker ... for the records, this is continued on #1010. |
4 participants
Changes proposed in this pull request
This code is adding preconditioning capabilities to Cantera/CVODEs solver interface for reactor networks. It is focused on a specific type of preconditioning but the code is being written with extensibility in mind. It adds a "Preconditioners" file to the numerics portion of cantera which contains a "PreconditionerBase" that is used to write future preconditioners. It also contains a class derived from this called "AdaptivePreconditioner" which is the aforementioned specific preconditioner.
There are also a couple of functions added to specific reactors. IdealGasReactor is one example. Currently, I added a function to evaluate the energy equation which is also done inside of evalEqs but I wanted to avoid touching core Cantera code as much as possible. In this future, I would like to collaborate with the appropriate people to stream line this.
Checklist
scons build&scons test) and unit tests address code coverage