Cantera / cantera

Chemical kinetics, thermodynamics, and transport tool suite
https://cantera.org
Other
582 stars 342 forks source link

Deprecate StFlow::evalResidual #1619

Closed ischoegl closed 1 week ago

ischoegl commented 9 months ago

Changes proposed in this pull request

This is a follow-up to #1595, as proposed here: https://github.com/Cantera/cantera/pull/1595#issuecomment-1712494990

Other than renaming / reordering content and reintroducing legacy code for deprecation purposes, this PR does not introduce any new capabilities.

Checklist

ischoegl commented 9 months ago

@speth ... this is the PR I had mentioned earlier. The diff is a little deceiving, as git doesn't acknowledge that files were renamed/removed and only partially re-added - the detailed commit history makes this a little clearer.

@wandadars ... this PR reorders the updateXYZ methods as you had envisioned.

codecov[bot] commented 9 months ago

Codecov Report

Attention: Patch coverage is 81.85291% with 190 lines in your changes missing coverage. Please review.

Project coverage is 72.80%. Comparing base (b4fa531) to head (0e7e224). Report is 2 commits behind head on main.

Files Patch % Lines
src/oneD/Flow1D.cpp 84.86% 69 Missing and 43 partials :warning:
src/oneD/StFlow.cpp 59.84% 43 Missing and 10 partials :warning:
src/clib/ctonedim.cpp 52.00% 24 Missing :warning:
src/oneD/IonFlow.cpp 85.71% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1619 +/- ## ========================================== - Coverage 72.84% 72.80% -0.05% ========================================== Files 379 381 +2 Lines 53824 54011 +187 Branches 9182 9207 +25 ========================================== + Hits 39208 39321 +113 - Misses 11642 11706 +64 - Partials 2974 2984 +10 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ischoegl commented 9 months ago

@speth and @wandadars : I am closing this as there is too much interference with #1622.

speth commented 9 months ago

I think this is still worth pursuing in general, but waiting until some of the other changes @wandadars is working on are finished makes sense to me.

ischoegl commented 8 months ago

I believe the window of opportunity to resolve this in 3.1 is de facto closed (see also #1639).

ischoegl commented 3 months ago

Reopening so this can be tracked - I'd like to revisit as long as #1622 can be resolved prior to 3.1

wandadars commented 3 months ago

I have been testing the implementation against cases that I have solved using FlameMaster. So far I have been able to run an RCM1 H2/O2 ideal gas case using the "flamelets" generated by the two-point control method. I think that's a pretty good test of the implementation, so I will be making a push to merge that branch starting this week.

ischoegl commented 1 week ago

With #1622 being merged, I hope to rebase these changes to current main, so we can get those into 3.1.

ischoegl commented 1 week ago

Rebased.

PS: As mentioned on top, the PR is actually much smaller than indicated by the line count (renaming + back port is an unfortunate combination).