Non-empty Reactor and Connector Objects by ischoegl · Pull Request #1788 · Cantera/cantera

ischoegl · 2024-08-23T03:57:46Z

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 · 2024-08-23T18:23:47Z

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 334 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 · 2024-08-25T04:51:23Z

@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 · 2024-08-27T02:52:34Z

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.

include/cantera/zeroD/ReactorNet.h

include/cantera/zeroD/ReactorFactory.h

include/cantera/zeroD/Reactor.h

include/cantera/zeroD/ReactorBase.h

samples/cxx/combustor/combustor.cpp

interfaces/cython/cantera/reactor.pyx

speth · 2024-09-10T13:48:34Z

include/cantera/zeroD/ReactorBase.h

    //! Set the Solution specifying the ReactorBase content.
    //! @param sol  Solution object to be set.
    //! @since New in %Cantera 3.1.
+    //! @deprecated  To be removed after %Cantera 3.1. Superseded by instantiation of
+    //!              ReactorBase with Solution object.
    void setSolution(shared_ptr<Solution> sol);


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

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?

See Cantera/enhancements#201 (comment)

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.

test/python/test_reactor.py

speth · 2024-09-10T14:58:51Z

src/clib/ctreactor.cpp

    {
        try {
-            return ReactorCabinet::at(i)->mass();
+            return ReactorCabinet::as<ReactorBase>(i)->mass();


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

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.

samples/python/reactors/mix1.py

The newReactor function with contents was only introduced in 3.1 and thus can be modified without deprecations.

ischoegl · 2024-09-15T17:13:27Z

@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 · 2024-10-13T14:16:22Z

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 · 2024-10-13T16:36:09Z

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.

ischoegl · 2025-02-04T03:58:50Z

@speth ... I pulled out the less controversial commits to #1848; I also added some comments to Cantera/enhancements#213.

ischoegl added Python Matlab Reactors clib labels Aug 23, 2024

ischoegl force-pushed the zerod-housekeeping branch 2 times, most recently from b3dd2d1 to d295959 Compare August 23, 2024 18:03

ischoegl force-pushed the zerod-housekeeping branch 13 times, most recently from 6fcbd68 to 7ae226a Compare August 25, 2024 04:21

ischoegl marked this pull request as ready for review August 25, 2024 04:51

ischoegl requested a review from a team August 25, 2024 04:51

ischoegl force-pushed the zerod-housekeeping branch 3 times, most recently from a05281f to 706861f Compare September 1, 2024 19:50

speth requested changes Sep 10, 2024

View reviewed changes

speth mentioned this pull request Sep 10, 2024

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

Closed

ischoegl mentioned this pull request Sep 10, 2024

Identify flow devices and walls in graphviz reactor network illustrations Cantera/enhancements#212

Closed

ischoegl added 14 commits September 15, 2024 09:22

[Python] Remove _install methods

d3b1bf0

[clib] Add new Connector functions

6ec380a

[MATLAB] Use CLib connector functions

7b8be61

[clib] Remove obsolete functions

5777803

[sourcegen] Add Connector crosswalk

af1e8d6

[zeroD] Update deprecation warnings

21205c0

[zeroD] Move syncState to ReactorNode

add6b91

[samples] Add plt.show() to Python samples

60a9ed9

Change return type of newReactor with contents

e108bae

The newReactor function with contents was only introduced in 3.1 and thus can be modified without deprecations.

[base] Implement SharedFactory

313e78d

[zeroD] Make Connector constructors protected

4cc6e77

[zeroD] Switch ReactorFactory to shared pointers

b61c7b2

[zeroD] Protect Reactor constructors

fbcd9e9

[zeroD] Deprecate methods using raw pointers

c923449

ischoegl force-pushed the zerod-housekeeping branch from 1749566 to e51053c Compare September 15, 2024 16:09

ischoegl added 3 commits September 15, 2024 11:42

[zeroD] Access reactor contents via ReactorNode

d5e6751

[CI] Update zeroD googletests

f778d3e

[samples] Update cxx samples

c17b309

ischoegl force-pushed the zerod-housekeeping branch from e51053c to c17b309 Compare September 15, 2024 16:44

ischoegl marked this pull request as ready for review September 15, 2024 17:06

ischoegl requested a review from speth September 15, 2024 18:05

ischoegl marked this pull request as draft October 13, 2024 16:37

ischoegl mentioned this pull request Feb 4, 2025

Zerod connector nodes #1848

Merged

5 tasks

ischoegl closed this Feb 4, 2025

ischoegl mentioned this pull request Mar 24, 2025

Update ReactorSurface / generalize ReactorBase #1864

Merged

5 tasks

ischoegl deleted the zerod-housekeeping branch June 3, 2025 16:44

Uh oh!

Conversation

ischoegl commented Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ischoegl commented Aug 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ischoegl commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

speth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

speth Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

ischoegl Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

speth Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

ischoegl Sep 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

speth Sep 10, 2024

Choose a reason for hiding this comment

Uh oh!

ischoegl Sep 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ischoegl commented Sep 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

speth commented Oct 13, 2024

Uh oh!

ischoegl commented Oct 13, 2024

Uh oh!

ischoegl commented Feb 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ischoegl commented Aug 23, 2024 •

edited

Loading

codecov bot commented Aug 23, 2024 •

edited

Loading

ischoegl commented Aug 25, 2024 •

edited

Loading

ischoegl commented Aug 27, 2024 •

edited

Loading

ischoegl Sep 15, 2024 •

edited

Loading

ischoegl commented Sep 15, 2024 •

edited

Loading