Conversation
|
I don't know how to edit the PR header, but this isn't a WIP anymore. |
speth
changed the title
|
Based on the tests that are failing and timing out, it seems like this is affecting the solution of steady-state reactor networks, which also use the |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2074 +/- ##
==========================================
+ Coverage 77.53% 77.63% +0.09%
==========================================
Files 451 451
Lines 52810 53150 +340
Branches 8828 8868 +40
==========================================
+ Hits 40947 41261 +314
- Misses 8888 8909 +21
- Partials 2975 2980 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ng controls, add tests for new options
wandadars
force-pushed
the
timestep_regridding
branch
from
bbd68d4 to
0fe8f72
Compare
speth
left a comment
speth
left a comment
There was a problem hiding this comment.
Thanks for putting this together, @wandadars. I think there are some useful ideas here. What we need now is something to show that. If there are any very simple cases where this can be exercised, it would be great to add them to the test suite. Besides that, we should add a new example demonstrating these options and cases where they have a beneficial impact on solver convergence (and perhaps when they don't).
| //! Enable or disable adaptive time-step growth heuristics. | ||
| //! | ||
| //! Adaptive heuristics are only applied after successful timesteps that reuse the | ||
| //! Jacobian. If disabled, the fixed growth factor from setTimeStepGrowthFactor() | ||
| //! is used directly. Disabled by default. | ||
| //! | ||
| //! @param enabled Enable (`true`) or disable (`false`) adaptive growth heuristics. | ||
| void setAdaptiveTimeStepGrowth(bool enabled) { | ||
| m_adaptive_tstep_growth = enabled; | ||
| } |
| heuristic); | ||
| } | ||
| m_tstep_growth_heuristic = heuristic; | ||
| } |
There was a problem hiding this comment.
- I think the heuristics should be specified by a name rather than an integer. So, something like
steady-norm,transient-residual, etc. - Besides an example case where each of these is effective, these definitely need at least some non-trivial tests, like an actual solve where they are active.
| //! Determine the timestep growth factor after a successful step. | ||
| //! | ||
| //! Called only when a successful step reuses the current Jacobian. | ||
| double timeStepIncreaseFactor(span<const double> x_before, | ||
| span<const double> x_after); |
There was a problem hiding this comment.
This name invites confusion with the property timeStepGrowthFactor. Perhaps calculateTimeStepGrowthFactor?
| //! If `true`, use heuristic gating for successful-step growth; otherwise use the | ||
| //! fixed growth factor. | ||
| bool m_adaptive_tstep_growth = false; | ||
|
|
There was a problem hiding this comment.
You could define this as another growth factor case, "fixed-growth" as opposed to having a separate variable.
| if (loglevel == 1) { // At level 1, output concise information | ||
| double ss = ssnorm(x, r); | ||
| writelog("\n{:<5d} {:<6.4e} {:>7.4f}", n, dt, log10(ss)); | ||
| } else if (loglevel > 1) { | ||
| double ss = ssnorm(x, r); | ||
| writelog("\nTimestep ({}) dt= {:<11.4e} log(ss)= {:<7.4f}", n, dt, log10(ss)); | ||
| if (loglevel >= 1) { | ||
| double ss_before = ssnorm(x, r); | ||
| if (loglevel == 1) { // At level 1, output concise information | ||
| writelog("\n{:<5d} {:<6.4e} {:>7.4f}", n, dt, log10(ss_before)); | ||
| } else { | ||
| writelog("\nTimestep ({}) dt= {:<11.4e} log(ss)= {:<7.4f}", n, dt, | ||
| log10(ss_before)); | ||
| } |
There was a problem hiding this comment.
Despite the separate calls to ssnorm in each branch, the previous version of this was more concise.
| double grow_factor = 1.0; | ||
| if (m_jac->nEvals() == j0) { | ||
| dt *= 1.5; | ||
| grow_factor = timeStepIncreaseFactor(x, r); | ||
| } | ||
| copy(r.begin(), r.end(), x.begin()); | ||
| dt *= grow_factor; |
There was a problem hiding this comment.
Simpler, and with a more minimal diff:
| double grow_factor = 1.0; | |
| if (m_jac->nEvals() == j0) { | |
| dt *= 1.5; | |
| grow_factor = timeStepIncreaseFactor(x, r); | |
| } | |
| copy(r.begin(), r.end(), x.begin()); | |
| dt *= grow_factor; | |
| if (m_jac->nEvals() == j0) { | |
| dt *= timeStepIncreaseFactor(x, r); | |
| } | |
| copy(r.begin(), r.end(), x.begin()); |
…here a remove only grid
|
Thanks for the updates, @wandadars. I just wanted to check, is this ready for another review? |
|
Yes @speth I think so. Sorry for the delay. |
3 participants
During a discussion on the users group, Ray mentioned that Cantera doesn't perform any regridding if timestepping fails during a solve. This pull request is an attempt to add a capability to the 1D solver that would allow for a regrid to happen if there is a failure in the timestepping. This feature allows for a set number of regridding steps to be taken during a failure of the timestepping. The test case that we were looking at was that of a pressure field being stepped up incrementally on a counterflow diffusion flame domain. The highest pressure possible was increased when the timestep regridding is allowed. This happens when cases have a bad initial condition or a coarse grid and fails right at the start during the timestepping phase.
This is Ray's script that I tweaked a bit and used to test the regridding.
Details
AI Statement (required)
-Extensive use of generative AI.
Significant portions of code or documentation were generated with AI, including
logic and implementation decisions. All generated code and documentation were
reviewed and understood by the contributor. Examples: Output from agentic coding
tools and/or substantial refactoring by LLMs (web-based or local).
Checklist
scons build&scons test) and unit tests address code coverage