Deprecate creation of empty `Wall` or `FlowDevice` objects in C++

[ischoegl]

Abstract

Creation of empty Wall, FlowDevice and ReactorSurface objects is not supported by the Python API, and Cantera/cantera#1765 removes preliminary support from experimental MATLAB. It would be consistent to disallow this at the C++ level as well, which would (eventually) simplify/streamline the interface by removing several methods and associated exception handling. Passing shared pointers to ReactorBase objects to constructors instead would be a significant step towards elimination of raw pointers from the reactor code base. A similar argument holds for ReactorNet and the pending ReactorEdge (see Cantera/cantera#1697).

Motivation

Describe the need for the proposed change:

• What problem is it trying to solve? ... simplify reactor API
• Who is affected by the change? ... mostly developers
• Why is this a good solution? ... simplifies API

Possible Solutions

Update constructors and factory methods and clib beyond changes introduced in Cantera/cantera#1765. Deprecate various install methods. Constructors should be updated to use signatures similar to

FlowDevice(shared_ptr<ReactorBase> upstream=nullptr, shared_ptr<ReactorBase> downstream=nullptr, 
           const string& name="(none)")

where null upstream/downstream will raise deprecation warnings in 3.1, with defaults being removed thereafter.

References

• Update ReactorSurface / generalize ReactorBase cantera#1864
• Zerod connector nodes cantera#1848
• Non-empty Reactor and Connector Objects cantera#1788
• Reproducible names for zeroD objects cantera#1765
• Implementation of 'ReactorEdge' class cantera#1697

[ischoegl]

@speth … could you have a look at Cantera/cantera#1765, so I can move ahead and tackle this (as well as deprecate a bunch of raw pointered interfaces) prior to the release of 3.1?

[speth]

(repeating some comments from my review of Cantera/cantera#1788 here, since I think this is a better place for discussion)

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

[ischoegl]

Thanks for the review of Cantera/cantera#1788 - before I address change requests, I wanted to briefly follow up on the shared_ptr issue. At the moment, whatever is 'under the hood' of various 0D objects in the current implementation is a mix of references, raw pointers, etc., which I was hoping to replace with a more consistent approach once the interfaces allow for it. It is true that there is the issue of cyclical references that need to be avoided. At the same time, I am not sure what the alternative would be if we want to stop using raw pointers - giving ReactorNet shared ownership of an object would imho create a clear structure, which would be safer than relying on the existence of externally managed objects. I personally see the external API only as a way to instantiate objects, with the C++ core having the requirement to always handle things safely.

Regarding the implementation under the hood, I think we need to increase the importance of ReactorNet in handling interactions, so we can avoid the doubly-linked list issues you point out. From that perspective, Connectors need to know what's in their adjacent Reactor nodes (i.e. a shared_ptr would be nice to have), but Reactor objects only need to know fluxes when integrating their governing equations. The calculation could be done in two steps: (1) evaluate connectors and build a global source term, and (2) evaluate governing equations. While this would a shift in paradigms, I believe it would help with #202. This is admittedly not a completely thought-through approach, but I ultimately believe that we need to get away from raw pointers and references to future-proof the code base.

[ischoegl]

For further thoughts on anticipated implementations see #201 (comment)

[ischoegl]

@speth ... before revisiting Cantera/cantera#1788 (or, likely, a sequence of smaller PRs as there now is a rather large number of merge conflicts), I wanted to discuss one of the points you raise Cantera/cantera#1788 (review):

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

I think this is an interesting point, but I ultimately think that we need to better differentiate between implementation and visualization. I am partial to the idea of approaching this from the perspective of a 'bipartite graph' with Node and Connector objects having very specific (and separate!) meanings. There is a wrinkle that this approach has some limitations with regards to the current GraphViz/dot approach that interprets 'connector' objects as 'edges': connectors interfacing with three nodes are not easily visualized (example: EdgeReactor as envisioned by Cantera/cantera#1697). Edit: upon further consideration, there isn't an issue, as EdgeReactor would be a ReactorNode.

Re-reading what I just wrote I realize that it's probably the terminology that is to blame: in the bipartite graph of Cantera/cantera#1788, the two types of objects are ReactorNode and Connector, where a connector is de facto a node and not an edge. I'd suggest to use the following nomenclature:

• ReactorNode (same name as in Non-empty Reactor and Connector Objects cantera#1788): essentially a refactored ReactorBase that absorbs ReactorSurface (currently classified as a Wall Edit: it is misclassified; it is not derived from WallBase.)
• ConnectorNode (clarified from Connector): a new base class that joins Wall and FlowDevice objects

I hope that this updated terminology will avoid erroneously conflating 'connectors' and 'edges'.

• ReactorNetwork: $ G = (R ∪ C, E) $
• Reactors: $ R1, R2, \dots, R_n $
• Connectors: $ C1, C2, \dots, C_m $
• Edges: $ e_{ij} = (R_i, C_j) $