Cantera / cantera

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

Remove/deprecate legacy Solution handling #1685

Closed ischoegl closed 2 months ago

ischoegl commented 3 months ago

Changes proposed in this pull request

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

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

codecov[bot] commented 3 months ago

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.

:exclamation: 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 :warning:
src/zeroD/IdealGasConstPressureMoleReactor.cpp 66.66% 1 Missing :warning:
src/zeroD/IdealGasConstPressureReactor.cpp 66.66% 1 Missing :warning:
src/zeroD/IdealGasMoleReactor.cpp 66.66% 1 Missing :warning:
src/zeroD/IdealGasReactor.cpp 66.66% 1 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

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

ischoegl commented 3 months ago

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

ischoegl commented 2 months ago

@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.