Non-empty Reactor and Connector Objects

[ischoegl]

Changes proposed in this pull request

This PR ensures that the Reactor/zeroD API uses shared pointers consistently. After removing raw pointers/references from various methods, it becomes possible to update the underlying code base, which at this point is a mix of shared pointers, raw pointers, and references. Details are:

• Create two new bases ReactorNode and Connector, which are bases for all 0D objects (which from the perspective of a graph are either nodes or edges); all objects are captured in updated factories.
  • ReactorBase and ReactorSurface are now derived from ReactorNode.
  • Wall and FlowDevice are both derived from Connector.
• All ReactorNode and Connector objects require contents to be available to the constructor (other than deprecated versions)
• Cantera 3.0 still had default constructors for all 0D objects (without parameters). Additions thus far implemented for 3.1 are moved to protected constructors, with new convenience methods creating objects accessed by shared pointers (examples: newReservoir and newValve). The old constructors (without arguments) are deprecated. The implementation loosely follows newSolution. ... Edit: constructors are accessed via Reservoir::create, Valve::create etc..
• A new SharedFactory factory variant stores shared pointers rather than raw pointers.
• The approach is motivated by future use of shared_from_this when connecting a ReactorNet.
• Changes are propagated to all relevant API’s, i.e. Python, CLib and MATLAB.
• Add wall names to ReactorNet graphviz illustrations. Edit: Moved to Graphviz tweaks #1792

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

[codecov]

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.

Files with missing lines	Patch %	Lines
src/clib/ctreactor.cpp	45.45%	30 Missing ⚠️
src/zeroD/ReactorSurface.cpp	30.00%	9 Missing and 5 partials ⚠️
src/zeroD/ConnectorFactory.cpp	72.22%	10 Missing ⚠️
src/zeroD/ReactorFactory.cpp	70.96%	9 Missing ⚠️
include/cantera/base/FactoryBase.h	56.25%	7 Missing ⚠️
src/zeroD/ReactorNode.cpp	69.56%	6 Missing and 1 partial ⚠️
interfaces/cython/cantera/reactor.pyx	90.16%	6 Missing ⚠️
src/zeroD/ReactorNet.cpp	76.19%	3 Missing and 2 partials ⚠️
include/cantera/zeroD/ReactorNode.h	66.66%	4 Missing ⚠️
include/cantera/zeroD/ReactorBase.h	0.00%	3 Missing ⚠️
... and 8 more

Additional details and impacted files

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

[ischoegl]

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

[ischoegl]

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]

I think the changes to have Reactor objects always contain a valid Solution object make sense, and I agree that this will help with Cantera/enhancements#201. I also like the idea of always providing the Reactor objects to the constructor for the connector objects and bringing that feature of the Python API to all the interfaces.

However, it's not quite clear to me what the benefit is of making all Reactor and connector objects into mandatory shared_ptrs. Currently, the ownership model is simple: none of the Reactor/ReactorNet/FlowDevice/etc. objects take ownership of anything they are connected to; the lifetimes of all these objects are managed by the caller. Within this set of objects, we need connections in all directions. Avoiding reference cycles will require the use of weak_ptr for many of the connections, with the associated overhead of "upgrading" those to shared_ptr where they are used. And you will still be left with the situation where it's possible to hold on to some element of the network but not be able to use it because the rest of the network has been deleted, though at least that would be checked and generate an exception rather than undefined behavior. My apologies for not thinking this through earlier and discussing it on Cantera/enhancements#213 before you put the time into implementing it.

Regarding the extension of the graph idiom, I think there's a simplification that may be worth considering. Instead of seeing just Reactor and ReactorSurface objects as nodes in the graph, you could also see valves and flow devices as nodes, with the edges of the graph just being the abstract connections among all of the entities that make up the reactor network. This way, there's only the need for one new base class, and no need for anything to sit between a Reactor and a ReactorSurface. This also gets closer to the original intent of Cantera/enhancements#212, and may work better as the amount of information you might want to display about a connector increases, given some of the limitations of how Graphviz handles (or doesn't) text associated with edges. That said, I do like the Connector class as a generalization combining walls and flow devices, and there's probably some further room to adjust this idea.

[speth]

Now that I think about this more, my preferred resolution to Cantera/enhancements#201 will require allowing ReactorNet to replace Solution objects in Reactors.

[ischoegl]

Could you expand? My thinking was a strict hierarchy where the ReactorNet is responsible for Reactor, which themselves are responsible for Solution. I am not sure that I see why we should give ReactorNet the ability to muddle with reactor contents?

[speth]

See Cantera/enhancements#201 (comment)

[ischoegl]

I responded in Cantera/enhancements#201 (comment). Irrespective of the logic we go with eventually, the detection of previously used Solution objects should be handled by ReactorNode and not ReactorNet.

[speth]

The fact that almost all usage of ReactorNode objects requires downcasting to ReactorBase, ReactorSurface, or Reactor suggests to me that we shouldn't be defining our interface in terms of ReactorNode objects (which is not to say that the base class isn't useful for implementing some features like the automatic name generation).

[ischoegl]

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.

[ischoegl]

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

[speth]

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.

[ischoegl]

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.