Skip to content

Update ReactorSurface / generalize ReactorBase#1864

Merged
speth merged 16 commits intoCantera:mainfrom
ischoegl:update-reactor-surface
Apr 23, 2025
Merged

Update ReactorSurface / generalize ReactorBase#1864
speth merged 16 commits intoCantera:mainfrom
ischoegl:update-reactor-surface

Conversation

@ischoegl
Copy link
Copy Markdown
Member

@ischoegl ischoegl commented Mar 24, 2025 •
edited
Loading

Changes proposed in this pull request

This PR pulls in suggestions from #1788, while trying to avoid unnecessary churn.

Changes are made in anticipation of the experimental CLib zeroD interface per Cantera/enhancements#220; other references are Cantera/enhancements#202 and Cantera/enhancements#61.

The resulting inheritance diagram is:
image

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

Closes Cantera/enhancements#213

If applicable, provide an example illustrating new features this pull request is introducing

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

@ischoegl ischoegl force-pushed the update-reactor-surface branch from ee57499 to b371bdf Compare March 24, 2025 01:44
@ischoegl ischoegl mentioned this pull request Feb 4, 2025
Closed
@ischoegl ischoegl force-pushed the update-reactor-surface branch from b371bdf to cd5e719 Compare March 24, 2025 02:03
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 24, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 51.42857% with 51 lines in your changes missing coverage. Please review.

Project coverage is 74.34%. Comparing base (3f2057c) to head (ebdd610).

Files with missing lines Patch % Lines
src/zeroD/ReactorSurface.cpp 45.71% 15 Missing and 4 partials ⚠️
include/cantera/zeroD/ReactorSurface.h 18.18% 9 Missing ⚠️
include/cantera/zeroD/ReactorBase.h 11.11% 8 Missing ⚠️
...faces/matlab_experimental/Reactor/ReactorSurface.m 0.00% 5 Missing ⚠️
src/clib/ctreactor.cpp 0.00% 3 Missing ⚠️
src/zeroD/Reactor.cpp 76.92% 1 Missing and 2 partials ⚠️
src/zeroD/ReactorBase.cpp 66.66% 2 Missing ⚠️
include/cantera/zeroD/Reservoir.h 0.00% 1 Missing ⚠️
src/zeroD/ReactorFactory.cpp 66.66% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1864      +/-   ##
==========================================
- Coverage   74.36%   74.34%   -0.02%     
==========================================
  Files         443      443              
  Lines       55407    55391      -16     
  Branches     9105     9109       +4     
==========================================
- Hits        41201    41181      -20     
- Misses      11108    11111       +3     
- Partials     3098     3099       +1

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ischoegl ischoegl marked this pull request as ready for review March 24, 2025 02:34
@ischoegl ischoegl requested a review from a team March 24, 2025 02:35
@ischoegl ischoegl marked this pull request as draft March 30, 2025 17:46
@ischoegl ischoegl removed the request for review from a team March 30, 2025 19:18
@ischoegl ischoegl force-pushed the update-reactor-surface branch 3 times, most recently from 17c9051 to efd5aa4 Compare March 31, 2025 13:34
@ischoegl ischoegl mentioned this pull request Mar 31, 2025
Closed
5 tasks
@ischoegl ischoegl changed the title Update ReactorSurface Update ReactorSurface / generalize ReactorBase Mar 31, 2025
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Mar 31, 2025

@speth ... I decided to revise this PR once more as I was unsatisfied with the previous version. At this point, the name of the base class remains ReactorBase (my preference ReactorNode would create too much churn). In addition, I disabled undefined operations, where associated properties remain zero if queried from ReactorBase (examples: volume for Reservoir or ReactorSurface is zero as the ReactorBase::setInitialVolume now requires an override to be implemented; similarly, ReactorSurface cannot add inlets/outlets, etc.).

@ischoegl ischoegl marked this pull request as ready for review March 31, 2025 15:12
@ischoegl ischoegl requested a review from a team March 31, 2025 15:12
@ischoegl ischoegl force-pushed the update-reactor-surface branch 4 times, most recently from 881a78d to 6e8cb37 Compare April 14, 2025 13:59
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Apr 14, 2025

Rebased after merge of #1665.

speth
speth requested changes Apr 21, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks, @ischoegl for moving ahead with this approach. I'm not sure I fully agree with my previous suggestion to have ReactorSurface derive from ReactorBase, but at least this is not too invasive, and I think there is a route to eventually resolving some of the oddities that this introduces. I had just a few suggestions before we merge this.

@ischoegl ischoegl force-pushed the update-reactor-surface branch from 6e8cb37 to ebdd610 Compare April 21, 2025 21:20
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Apr 21, 2025 •
edited
Loading

Thanks, @speth ... I addressed both comments, so this PR should be ready for a merge.

Regarding oddities: yes, I am aware of some. For ReactorSurface, a lot of the details depend on how we resolve Cantera/enhancements#202.

@speth speth merged commit 0eb7f94 into Cantera:main Apr 23, 2025
47 of 49 checks passed
@ischoegl ischoegl deleted the update-reactor-surface branch April 23, 2025 18:55
@speth speth mentioned this pull request Jun 7, 2025
Closed
@ischoegl ischoegl mentioned this pull request Jun 21, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@speth speth speth approved these changes

Assignees

No one assigned

Labels

clib Matlab Python Reactors

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Deprecate creation of empty Wall or FlowDevice objects in C++

2 participants

@ischoegl @speth