Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate creation of empty Wall or FlowDevice objects in C++ #213

Open
ischoegl opened this issue Aug 10, 2024 · 4 comments · May be fixed by Cantera/cantera#1788
Open

Deprecate creation of empty Wall or FlowDevice objects in C++ #213

ischoegl opened this issue Aug 10, 2024 · 4 comments · May be fixed by Cantera/cantera#1788
Assignees
Labels
work-in-progress An enhancement that someone is currently working on

Comments

@ischoegl
Copy link
Member

ischoegl commented Aug 10, 2024

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

@ischoegl ischoegl added the feature-request New feature request label Aug 10, 2024
@ischoegl ischoegl self-assigned this Aug 10, 2024
@ischoegl
Copy link
Member Author

ischoegl commented Aug 18, 2024

@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
Copy link
Member

speth commented Sep 10, 2024

(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
Copy link
Member Author

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 ischoegl added work-in-progress An enhancement that someone is currently working on and removed feature-request New feature request labels Sep 15, 2024
@ischoegl
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work-in-progress An enhancement that someone is currently working on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants