Skip to content

Toward consistent naming of reactor "contents"#2001

Merged
ischoegl merged 4 commits intoCantera:mainfrom
speth:rename-reactor-contents
Oct 9, 2025
Merged

Toward consistent naming of reactor "contents"#2001
ischoegl merged 4 commits intoCantera:mainfrom
speth:rename-reactor-contents

Conversation

@speth
Copy link
Copy Markdown
Member

@speth speth commented Oct 8, 2025 •
edited
Loading

Changes proposed in this pull request

Python:

  • Add new contents property for ReactorBase objects that returns a Solution object
  • Deprecate old thermo and kinetics properties of ReactorBase, Reactor, and ReactorSurface

C++:

  • Deprecate the existing ReactorBase::contents method that returns ThermoPhase&
  • Rename the solution property of ReactorBase (which returns shared_ptr<Solution> to contents4, with the expectation of renaming this contents in a future version
  • Change the implementation of ReactorSurface::syncState which actually did the opposite of the ReactorBase::syncState method. I don't think there's a non-breaking way to implement this change, but I also don't think there is much if any user code that calls this method directly. Normally, users would rely on Reactor methods to do this synchronization.

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

@speth speth changed the title Toward constent naming of reactor "contents" Toward consistent naming of reactor "contents" Oct 8, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 8, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 70.27027% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.41%. Comparing base (b614984) to head (02c54fb).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
interfaces/cython/cantera/reactor.pyx 60.00% 6 Missing ⚠️
src/zeroD/ReactorSurface.cpp 40.00% 3 Missing ⚠️
include/cantera/zeroD/ReactorBase.h 50.00% 1 Missing ⚠️
src/zeroD/ReactorNet.cpp 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2001      +/-   ##
==========================================
- Coverage   75.43%   75.41%   -0.03%     
==========================================
  Files         454      454              
  Lines       56536    56548      +12     
  Branches     9331     9331              
==========================================
- Hits        42650    42647       -3     
- Misses      10730    10746      +16     
+ Partials     3156     3155       -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.

@speth speth marked this pull request as ready for review October 8, 2025 21:20
ischoegl
ischoegl reviewed Oct 9, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

Thank you, @speth! I appreciate you implementing my suggestion.

@ischoegl ischoegl merged commit 1768cf6 into Cantera:main Oct 9, 2025
48 of 51 checks passed
@speth speth deleted the rename-reactor-contents branch October 9, 2025 12:04
@ischoegl ischoegl mentioned this pull request Oct 10, 2025
Open
8 tasks
@ischoegl
Copy link
Copy Markdown
Member

ischoegl commented Oct 10, 2025 •
edited
Loading

@speth ... thanks again for taking this on. Replacing Reactor.thermo with contents is a significant improvement, as now all objects are accessed via the associated Solution. Since I merged this PR, I looked into adopting the same nomenclature for Domain1D and SolutionArray. Based on what I saw, I am now wondering whether phase may not have been a better long-term name, as it will be less disruptive elsewhere. I realize that there is still an issue with Phase/ThermoPhase being used for handling of thermo properties on the C++ side. I.e., a switch would result in less disruption to user-facing APIs, albeit with some less consistent naming choices in the C++ codebase. (I'm also aware that not every ThermoPhase satisfies thermodynamic definitions of a Phase, which is an entirely different ball of hair).

Since I pushed on the current contents (and quickly merged), I would be happy to make the required updates myself. Please let me know.

@speth
Copy link
Copy Markdown
Member Author

speth commented Oct 10, 2025

Naming this accessor "phase" does have the benefit of eliminating the need for the transitional "contents4" name, as well as well as being the shortest practical name I can imagine.

@ischoegl
Copy link
Copy Markdown
Member

ischoegl commented Oct 10, 2025

Naming this accessor "phase" does have the benefit of eliminating the need for the transitional "contents4" name, as well as well as being the shortest practical name I can imagine.

Perfect - add the benefits on the side of SolutionArray and Domain1D, and it's a win-win. Will create a PR over the weekend.

@ischoegl ischoegl mentioned this pull request Oct 11, 2025
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ischoegl ischoegl ischoegl approved these changes

Assignees

No one assigned

Labels

Reactors

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@speth @ischoegl