Zerod connector nodes #1848

ischoegl · 2025-02-04T03:31:38Z

Changes proposed in this pull request

This PR pulls out the less controversial modifications from #1788, while improving the nomenclature (see Cantera/enhancements#213 (comment)):

Create a new ConnectorNode base, which combine Wall and FlowDevice objects; all objects are captured in updated factories.
Changes are propagated to all relevant API’s, i.e. Python, CLib and MATLAB.

If applicable, fill in the issue number this pull request is fixing

Addresses Cantera/enhancements#213

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 · 2025-02-04T04:13:25Z

Codecov Report

Attention: Patch coverage is 59.06433% with 70 lines in your changes missing coverage. Please review.

Project coverage is 74.35%. Comparing base (e6f3e9d) to head (c791052).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/clib/ctreactor.cpp	13.95%	37 Missing ⚠️
src/zeroD/ConnectorFactory.cpp	65.00%	12 Missing and 2 partials ⚠️
interfaces/cython/cantera/reactor.pyx	80.00%	6 Missing ⚠️
src/zeroD/FlowDevice.cpp	86.36%	3 Missing ⚠️
src/zeroD/Wall.cpp	66.66%	3 Missing ⚠️
include/cantera/zeroD/ConnectorNode.h	80.00%	2 Missing ⚠️
src/zeroD/ReactorBase.cpp	66.66%	2 Missing ⚠️
include/cantera/zeroD/FlowDevice.h	0.00%	1 Missing ⚠️
include/cantera/zeroD/Wall.h	0.00%	1 Missing ⚠️
src/zeroD/ConnectorNode.cpp	87.50%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1848      +/-   ##
==========================================
- Coverage   74.41%   74.35%   -0.06%     
==========================================
  Files         386      387       +1     
  Lines       53628    53631       +3     
  Branches     9063     9068       +5     
==========================================
- Hits        39905    39876      -29     
- Misses      10652    10686      +34     
+ Partials     3071     3069       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

speth

Thanks for separating this out, @ischoegl. I think introducing the ConnectorNode object type is useful and avoids some repetition. Broadly, though, the interface for the different derived types of connectors is different enough that I think the interface for using them should continue to be through objects of those particular types to avoid the need for dynamically casting to the derived types, both internally and for external users.

include/cantera/zeroD/ConnectorNode.h

speth · 2025-02-14T01:25:47Z

include/cantera/zeroD/ConnectorNode.h

+ * In a reactor network, walls and flow devices (valves, pressure regulators, etc.)
+ * form edges of a directed graph that connect reactors that form nodes.


This seems to get into the nomenclature question -- are connectors nodes (vertices) as the name ConnectorNode implies, or edges, as described here?

I corrected this based on the discussion in Cantera/enhancements#213 (comment)

speth · 2025-02-14T01:27:22Z

include/cantera/zeroD/ConnectorNode.h

+    //! Pair of reactors forming end points of the connector.
+    pair<shared_ptr<ReactorBase>, shared_ptr<ReactorBase>> m_nodes;


I don't think the pair adds much here -- two separate member variables ends up being easier to use.

I have a (slight?) preference for pair as it is a directed graph, where a ConnectorNode connects to exactly two reactors. As it is an implementation detail, we can always adjust at a later point.

test/zeroD/test_zeroD.cpp

speth · 2025-02-14T01:50:11Z

test/zeroD/test_zeroD.cpp

+    auto edge = newConnectorNode("Valve", node0, node1, "valve");
+    ASSERT_EQ(edge->name(), "valve");
+
+    auto valve = std::dynamic_pointer_cast<FlowDevice>(edge);
+    ASSERT_EQ(valve->in().name(), "upstream");
+    ASSERT_EQ(valve->out().name(), "downstream");
+


The fact that you can do almost nothing with a ConnectorNode without having to std::dynamic_pointer_cast it to a more derived type tells me that we should largely be hiding newConnectorNode from users. The preferred interface would then either be the newWall and newFlowDevice functions, or just expecting them to use make_shared<Wall>(...) and the like.

I created two new versions of newWall and newFlowDevice that use reactors as parameters and deprecated the old versions.

src/zeroD/Wall.cpp

src/zeroD/FlowDevice.cpp

speth · 2025-02-14T02:07:26Z

src/clib/ctreactor.cpp

        try {
-            return WallCabinet::at(i)->area();
+            return ConnectorCabinet::as<Wall>(i)->area();
        } catch (...) {
            return handleAllExceptions(DERR, DERR);


Especially as we move toward the generated clib, I'm not sure it's worth jamming more disparate types into the same Cabinet.

The generated CLib uses exactly one header file for all class specializations - all use the same Cabinet (at least in the current implementation, which of course may be refined as the project progresses). My plan for zeroD is to establish two node types - ConnectorNode and ReactorBase (potentially renamed ReactorNode for consistency). As ReactorNet is another required class, the CLib interface would be implemented using three headers.

fix doxygen for flowdeviceGroup

Add newWall and newFlowDevice with reactors as parameters, and deprecate old versions.

The C++ class does not inherit from WallBase.

ischoegl · 2025-02-16T17:38:37Z

@speth - Thank you for the review (and for catching some pre-3.1 remnants of #1788). I believe your comment regarding not restricting the interface to newConnectorNode is valid, so I added two updated versions that retain the original split between newWall and newFlowDevice. Regarding your CLib comment, the exact implementation will be fleshed out according to Cantera/enhancements#220, so I'd like to table this for the moment.

ischoegl added Python Reactors clib .NET labels Feb 4, 2025

ischoegl force-pushed the zerod-connector-nodes branch from 10b7038 to eefe4ca Compare February 4, 2025 03:51

This was referenced Feb 4, 2025

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

Open

Non-empty Reactor and Connector Objects #1788

Closed

ischoegl force-pushed the zerod-connector-nodes branch 5 times, most recently from 3ca0fab to 3822451 Compare February 4, 2025 14:50

ischoegl marked this pull request as ready for review February 4, 2025 16:01

ischoegl requested a review from a team February 4, 2025 16:01

speth requested changes Feb 14, 2025

View reviewed changes

ischoegl added 8 commits February 16, 2025 09:05

[zeroD] Implement Connector base class

f9578bb

[zeroD] Implement ConnectorFactory

9a9f378

[zeroD] Move newFlowDevice to ConnectorFactory

3eba650

[zeroD] Move newWall to ConnectorFactory

7659c1e

fix doxygen for flowdeviceGroup

[clib] Switch to ConnectorCabinet

f545527

[zeroD] Add deprecations

ad81e20

[zeroD] Install Reactors when instantiating connectors

7a6cc9a

[Python] Derive Wall/FlowDevice from ConnectorNode

b057dab

ischoegl force-pushed the zerod-connector-nodes branch from 3822451 to 967735b Compare February 16, 2025 16:49

ischoegl added 5 commits February 16, 2025 10:59

[zeroD] Add alternative Connector instantiations

d0e9622

Add newWall and newFlowDevice with reactors as parameters, and deprecate old versions.

[unittests] Improve test-zeroD coverage

27f4ff2

[clib] Add new Connector functions

bd5b2e2

[MATLAB] Use CLib connector functions

0ce3a7c

[clib] Remove obsolete functions

7809c73

ischoegl added 5 commits February 16, 2025 10:59

[sourcegen] Add Connector crosswalk

7bd0303

[zeroD] Remove ReactorSurface from wallGroup

1d749a2

The C++ class does not inherit from WallBase.

[zeroD] Deprecate Wall/FlowDevice factories

6d36d94

[Python] Clarifiy reactor content nomenclature

1d86759

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

c791052

ischoegl force-pushed the zerod-connector-nodes branch from 967735b to c791052 Compare February 16, 2025 16:59

ischoegl requested a review from speth February 16, 2025 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zerod connector nodes #1848

Zerod connector nodes #1848

ischoegl commented Feb 4, 2025 •

edited

Loading

codecov bot commented Feb 4, 2025 •

edited

Loading

speth left a comment

speth Feb 14, 2025

ischoegl Feb 16, 2025

speth Feb 14, 2025

ischoegl Feb 16, 2025

speth Feb 14, 2025

ischoegl Feb 16, 2025 •

edited

Loading

speth Feb 14, 2025

ischoegl Feb 16, 2025 •

edited

Loading

ischoegl commented Feb 16, 2025 •

edited

Loading

		* In a reactor network, walls and flow devices (valves, pressure regulators, etc.)
		* form edges of a directed graph that connect reactors that form nodes.

		//! Pair of reactors forming end points of the connector.
		pair<shared_ptr<ReactorBase>, shared_ptr<ReactorBase>> m_nodes;

Zerod connector nodes #1848

Are you sure you want to change the base?

Zerod connector nodes #1848

Conversation

ischoegl commented Feb 4, 2025 • edited Loading

codecov bot commented Feb 4, 2025 • edited Loading

Codecov Report

speth left a comment

Choose a reason for hiding this comment

speth Feb 14, 2025

Choose a reason for hiding this comment

ischoegl Feb 16, 2025

Choose a reason for hiding this comment

speth Feb 14, 2025

Choose a reason for hiding this comment

ischoegl Feb 16, 2025

Choose a reason for hiding this comment

speth Feb 14, 2025

Choose a reason for hiding this comment

ischoegl Feb 16, 2025 • edited Loading

Choose a reason for hiding this comment

speth Feb 14, 2025

Choose a reason for hiding this comment

ischoegl Feb 16, 2025 • edited Loading

Choose a reason for hiding this comment

ischoegl commented Feb 16, 2025 • edited Loading

ischoegl commented Feb 4, 2025 •

edited

Loading

codecov bot commented Feb 4, 2025 •

edited

Loading

ischoegl Feb 16, 2025 •

edited

Loading

ischoegl Feb 16, 2025 •

edited

Loading

ischoegl commented Feb 16, 2025 •

edited

Loading