Skip to content

Remove/deprecate legacy Solution handling#1685

Merged
speth merged 3 commits intoCantera:mainfrom
ischoegl:remove-legacy-methods
Apr 11, 2024
Merged

Remove/deprecate legacy Solution handling#1685
speth merged 3 commits intoCantera:mainfrom
ischoegl:remove-legacy-methods

Conversation

@ischoegl
Copy link
Copy Markdown
Member

@ischoegl ischoegl commented Mar 30, 2024 •
edited
Loading

Changes proposed in this pull request

Removal of legacy MATLAB toolbox #1670 allows for some simplifications / deprecations:

  • Remove remaining legacy wall methods from CAPI and C++
  • Deprecate setThermoMgr/setKineticsMgr for 0D
  • Move deprecation warning for empty reactor objects from Python to C++

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

Partially addresses #1457

A clean resolution of #1457 will require pushing the Python _WeakrefProxy approach to the C++ layer?

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 marked this pull request as draft March 30, 2024 17:50
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 30, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 88.09524% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 72.80%. Comparing base (ed27faf) to head (a8aae47).
Report is 1 commits behind head on main.

❗ Current head a8aae47 differs from pull request most recent head 3f20538. Consider uploading reports for the commit 3f20538 to get more accurate results

Files Patch % Lines
include/cantera/zeroD/ReactorBase.h 50.00% 0 Missing and 1 partial ⚠️
src/zeroD/IdealGasConstPressureMoleReactor.cpp 66.66% 1 Missing ⚠️
src/zeroD/IdealGasConstPressureReactor.cpp 66.66% 1 Missing ⚠️
src/zeroD/IdealGasMoleReactor.cpp 66.66% 1 Missing ⚠️
src/zeroD/IdealGasReactor.cpp 66.66% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1685      +/-   ##
==========================================
+ Coverage   72.76%   72.80%   +0.04%     
==========================================
  Files         378      378              
  Lines       56986    56956      -30     
  Branches    20691    20679      -12     
==========================================
+ Hits        41468    41469       +1     
+ Misses      12463    12434      -29     
+ Partials     3055     3053       -2

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

@ischoegl ischoegl marked this pull request as ready for review March 30, 2024 18:23
@ischoegl ischoegl mentioned this pull request Mar 30, 2024
Closed
@ischoegl ischoegl requested a review from a team March 30, 2024 18:34
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Mar 30, 2024 •
edited
Loading

Ready for review ... decided to restrict this PR to Reactor cleanup that became possible after the removal of the legacy MATLAB toolbox.

speth
speth requested changes Apr 3, 2024
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. I had a couple of thoughts on this.

Given the changes made here, can we move toward eliminating the C++ ReactorBase::setSolution and the Python ReactorBase.insert methods? With the requirement that the reactor needs contents from the beginning, I don't think there's a use case for switching them after the fact. (Not necessarily suggesting to resolve this in this PR)

@ischoegl ischoegl force-pushed the remove-legacy-methods branch from a8aae47 to a0c3f73 Compare April 7, 2024 13:53
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Apr 7, 2024 •
edited
Loading

@speth - thank you for your feedback. I believe I started some confusion with a deprecation warning that incorrectly implied that something was introduced in Cantera 3.0, while only being implemented after its release (non-empty Reactors). I updated/corrected the warnings, which should take care of your concerns.

Regarding changes to the reactor content after instantiation, we can definitely think about removing this capability, but either insert or setSolution are needed until the deprecation cycle is complete. I agree that any changes should be discussed in a separate PR.

@ischoegl ischoegl force-pushed the remove-legacy-methods branch from a0c3f73 to 3f20538 Compare April 7, 2024 16:58
@ischoegl ischoegl requested a review from speth April 9, 2024 12:19
speth
speth approved these changes Apr 11, 2024
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. These changes work for me.

@speth speth merged commit b80afd1 into Cantera:main Apr 11, 2024
@ischoegl ischoegl deleted the remove-legacy-methods branch August 19, 2024 12:48
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

Reactors

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ischoegl @speth