Conversation
ischoegl
force-pushed
the
zerod-connector-nodes
branch
from
10b7038 to
eefe4ca
Compare
Codecov ReportAttention: Patch coverage is
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. 🚀 New features to boost your workflow:
|
ischoegl
force-pushed
the
zerod-connector-nodes
branch
5 times, most recently
from
3ca0fab to
3822451
Compare
speth
left a comment
speth
left a comment
There was a problem hiding this comment.
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.
| * In a reactor network, walls and flow devices (valves, pressure regulators, etc.) | ||
| * form edges of a directed graph that connect reactors that form nodes. |
There was a problem hiding this comment.
This seems to get into the nomenclature question -- are connectors nodes (vertices) as the name ConnectorNode implies, or edges, as described here?
There was a problem hiding this comment.
I corrected this based on the discussion in Cantera/enhancements#213 (comment)
| //! Pair of reactors forming end points of the connector. | ||
| pair<shared_ptr<ReactorBase>, shared_ptr<ReactorBase>> m_nodes; |
There was a problem hiding this comment.
I don't think the pair adds much here -- two separate member variables ends up being easier to use.
There was a problem hiding this comment.
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.
fix doxygen for flowdeviceGroup
ischoegl
force-pushed
the
zerod-connector-nodes
branch
from
3822451 to
967735b
Compare
Add newWall and newFlowDevice with reactors as parameters, and deprecate old versions.
The C++ class does not inherit from WallBase.
ischoegl
force-pushed
the
zerod-connector-nodes
branch
from
967735b to
c791052
Compare
|
@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 |
|
@speth ... let me know if there's anything else that needs to be done before this can be merged. |
2 participants
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)):
ConnectorNodebase, which combineWallandFlowDeviceobjects; all objects are captured in updated factories.If applicable, fill in the issue number this pull request is fixing
Addresses Cantera/enhancements#213
Checklist
scons build&scons test) and unit tests address code coverage