-
-
Notifications
You must be signed in to change notification settings - Fork 361
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
Deprecate StFlow::evalResidual #1619
Conversation
1d51570
to
dbae71d
Compare
@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 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
dbae71d
to
128eff4
Compare
@speth and @wandadars : I am closing this as there is too much interference with #1622. |
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. |
I believe the window of opportunity to resolve this in 3.1 is de facto closed (see also #1639). |
Reopening so this can be tracked - I'd like to revisit as long as #1622 can be resolved prior to 3.1 |
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. |
With #1622 being merged, I hope to rebase these changes to current |
4b879f2
to
128eff4
Compare
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ischoegl. This looks good to me. Just a couple of small comments.
Also, add missing member variable documentation.
Changes proposed in this pull request
This is a follow-up to #1595, as proposed here: #1595 (comment)
StFlow
toFlow1D
(which is consistent with other class names used for 1D simulations)StFlow
with legacy methodsStFlow::evalResidual
, etc.; issue a deprecation warning when constructor is calledFlow1D
content and add new named sections to improve doxygen documentationtest-oneD
Other than renaming / reordering content and reintroducing legacy code for deprecation purposes, this PR does not introduce any new capabilities.
Checklist
scons build
&scons test
) and unit tests address code coverage