Open ischoegl opened 1 month ago
@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?
(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_ptr
s.
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.
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.
Abstract
Creation of empty
Wall
,FlowDevice
andReactorSurface
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 toReactorBase
objects to constructors instead would be a significant step towards elimination of raw pointers from the reactor code base. A similar argument holds forReactorNet
and the pendingReactorEdge
(see Cantera/cantera#1697).Motivation
Describe the need for the proposed change:
Possible Solutions
Update constructors and factory methods and
clib
beyond changes introduced in Cantera/cantera#1765. Deprecate variousinstall
methods. Constructors should be updated to use signatures similar towhere null
upstream
/downstream
will raise deprecation warnings in 3.1, with defaults being removed thereafter.References