Codecov Report

Attention: Patch coverage is 74.58034% with 106 lines in your changes missing coverage. Please review.

Project coverage is 73.33%. Comparing base (dd26e18) to head (c17b309).
Report is 9 commits behind head on main.

@@            Coverage Diff             @@
##             main    #1788      +/-   ##
==========================================
+ Coverage   73.23%   73.33%   +0.09%     
==========================================
  Files         381      389       +8     
  Lines       54376    54459      +83     
  Branches     9253     9270      +17     
==========================================
+ Hits        39824    39935     +111     
+ Misses      11579    11551      -28     
  Partials     2973     2973              

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

@speth ... while there are likely more paper cuts, I don't necessarily want to go beyond 2k lines.

Among the things that could be tackled are lowercase/hyphenated type names to be consistent with what we do with ThermoPhase and Kinetics, providing a name to ReactorNet, as well as merging Reactor.thermo/Reactor.kinetics into Reactor.contents in the Python API (Edit: in view of Cantera/enhancements#201, there may be better approaches). Those are relatively minor additions that could be done in a follow-up PR (if so desired). At the same time, there will be further updates in 3.2, although I'd like to keep those mostly under the hood.

I just realized that there are some connections to Cantera/enhancements#201 and Cantera/enhancements#202. While this PR doesn't address either directly, it does ensure that a resolution will become easier.

Without trying to pre-empt any eventual resolution, my personal take would be:

• Have each Reactor 'own' a copy of aSolution object exclusively to address Eliminate traps associated with accessing ThermoPhase objects in use by Reactors enhancements#201, which would likely require the implementation of a copy constructor for Solution. We then could attach Solution methods directly to ReactorNode, which would eliminate the need to access a thermo or kinetics member variable.
• Decouple ReactorSurface from bulk reactors to address Improve ReactorNet handling of bulk phase interactions enhancements#202. While this PR de facto declares ReactorSurface to be its own "node" already, I have not implemented an "edge" that ties a surface reactor to a bulk phase reactor. I was toying with the idea to replace ReactorSurface.install / ReactorBase::addSurface by new Connector specialization but ultimately decided against it. At the same time, it would be a logical next step.

@speth ... thank you again for your feedback. After further consideration, I decided to move ahead with revisions on this PR after all, as some of the API changes are necessary to tackle updates envisioned by various enhancement requests.

I adopted most of your suggestions, specifically:

• Reduce boilerplate, and use XYZ::create methods directly rather than newXYZ convenience functions.
• Remove functions previously marked as deprecated.
• Leave default constructors public.
• Moved ReactorNet illustration updates to a separate PR.

Beyond, I tried to clarify future plans that motivated API changes, see various comments in the contexts of Cantera/enhancements#201, Cantera/enhancements#202 and Cantera/enhancements#213.

Once the API is fixed, ReactorBase methods should be either shifted to ReactorNode or Reactor, with ReactorBase eventually being removed. From that perspective, this is transitional behavior.

Adding the following to ReactorBase.h:

 * @todo After completion of the %Cantera 3.1 deprecation cycle, all methods should be
 *      either moved to ReactorNode or Reactor, with ReactorBase entering its own
 *      deprecation cycle in %Cantera 3.2.

(moving out of a specific line-by-line comment to prevent GitHub from hiding this higher-level discussion)

I think this gets to my main concern with the API changes that this PR creates. Right now, there's a bit too much importance being placed on the idea of the reactor network as a graph -- most users don't care about this, but it now becomes the most important thing about a reactor and represents almost the only set of directly accessible functionality when using newReactorNode over the now-deprecated newReactor method.

I'd like to explore whether we can directly skip over creating the ReactorNode class and make ReactorBase serve this purpose immediately. The main difference that has with the current structure is that it would make ReactorSurface into a ReactorBase, but I think that's a step in the right direction anyway, given the other discussion we've had about the future of handling surfaces and edges in Cantera/enhancements#202. There aren't that many functions of ReactorBase that aren't applicable to a ReactorSurface and we can make those virtual functions and override them as needed.

I'd rather do this than leave users with an unnecessarily difficult-to-use interface for a full Cantera version.

Thanks for your response, @speth.

I think this gets to my main concern with the API changes that this PR creates. Right now, there's a bit too much importance being placed on the idea of the reactor network as a graph -- most users don't care about this, but it now becomes the most important thing about a reactor and represents almost the only set of directly accessible functionality when using newReactorNode over the now-deprecated newReactor method.

Interesting thought - my attempt to simplify was definitely influenced by viewing 'reactor networks as a graph', although my objective was to simplify/generalize rather than making things more complicated.

I'd like to explore whether we can directly skip over creating the ReactorNode class and make ReactorBase serve this purpose immediately. The main difference that has with the current structure is that it would make ReactorSurface into a ReactorBase, but I think that's a step in the right direction anyway, given the other discussion we've had about the future of handling surfaces and edges in Cantera/enhancements#202. There aren't that many functions of ReactorBase that aren't applicable to a ReactorSurface and we can make those virtual functions and override them as needed.

I agree that we have one superfluous layer, which needs to be addressed. I was thinking along the lines of differentiating between bulk and surface reactors. I still think that nodes and edges make sense overall, so my introduction of a new base class was a move to phase out ReactorBase while providing a little more flexibility.

I'd rather do this than leave users with an unnecessarily difficult-to-use interface for a full Cantera version.

I think at this point it makes sense to defer to 3.2. I will change the project planner accordingly.