Reactor net visualization

[Naikless]

Changes proposed in this pull request

This PR is based on the enhancement I suggested here. It includes functions and methods to directly visualize components and their connections in a ReactorNet or on their own using the grahpviz package.

I wrote the functionality as a standalone module and included the functions as methods in the corresponding objects. If this is considered too intrusive, skipping https://github.com/Cantera/cantera/commit/2ad20c41228c0c531f16a67fc28ec1a87c5de8ae leaves them separated.

Other things I would be happy to receive feedback about:

• For the sake of simplicity, the new cython extension class attributes for the draw options are currently implemented as Python dicts. If this is a performance issue at some point, they could probably also be something more low-level like strings.
• For the "lazy"/optional import of graphviz I have copied the behavior found for the Solution methods that require pandas. I don't know if this is still best practice.
• I don't know if I have sufficiently adhered to the preferred import pattern in __init__.py.
• unit tests are missing yet. Should they just be part of the TestReactor class?

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

Closes https://github.com/Cantera/enhancements/issues/180

If applicable, provide an example illustrating new features this pull request is introducing

• Adds functions and methods based on them that can draw ReactorBase, WallBase and FlowDevice objects

  gas = ct.Solution('h2o2.yaml')
  r = ct.Reactor(gas)
  r.draw()

  

  r2 = ct.Reactor(gas)
  w = ct.Wall(r,r2,U=0.1)
  w.draw()

  (not defining a phase for the reactors causes Wall to throw an Access Violation, as reported here)

• Reactors can be drawn plain or with their current state (T, P, composition) displayed

  r.draw()

  

  r.draw(print_state=True, species='X')

  

• Detects all connections between these objects and draws them as arrows. Corresponding mass and heat flows are added as labels. Connections between identical objects are summed up. Here is the visualization of the ic_engine.py sample:

  sim.draw()

  

• The style of these elements can be fully customized using graphviz regular node_attr and edge_attr keywords. This can be done either generally when calling the draw function or individually for each object by setting corresponding attributes, in which case the latter takes precedence:

  r.node_attr={"color":"red"}
  r.draw(node_attr={"color":"green", "shape":"square"})

  

Checklist

• [x] The pull request includes a clear description of this code change
• [x] Commit messages have short titles and reference relevant issues
• [x] Build passes (scons build & scons test)
• [x] unit tests address code coverage
• [x] Style & formatting of contributed code follows contributing guidelines
• [x] The pull request is ready for review

[Naikless]

Whoopsi, that's a lot of trailing whitespaces. Is it best practice to do a cleanup commit or should I try to rework them into the existing commits?

[codecov[bot]]

Codecov Report

Attention: 23 lines in your changes are missing coverage. Please review.

Comparison is base (87317b7) 72.68% compared to head (acfa842) 72.74%. Report is 3 commits behind head on main.

Files	Patch %	Lines
interfaces/cython/cantera/drawnetwork.py	86.06%	15 Missing and 8 partials :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1624 +/- ## ========================================== + Coverage 72.68% 72.74% +0.05% ========================================== Files 370 371 +1 Lines 56302 56500 +198 Branches 20403 20443 +40 ========================================== + Hits 40923 41100 +177 - Misses 12371 12385 +14 - Partials 3008 3015 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[bryanwweber]

Thank you! This is very cool

do a cleanup commit

A cleanup commit is fine 😁

[speth]

I think the issue is mainly that drawnetwork.py has Windows (CRLF) line endings, which triggers that warning on every line. If you're comfortable fixing that as part of a rebase, I think it would make the history a bit cleaner.

[Naikless]

Thanks for the encouragement and the review. I'll try get to it in the next days.

Just before I get started and not to over engineer this, but this is my first code review process: Is it preferable to address the requested changes as commits or as part of a rebase? Intuitively, I would try to incorporate trivial fixes like trailing whitespace into the original commits while creating new commits for stuff that might warrant a discussion/documentation.

And also to already address one of your suggestions @speth: I initially had the ..._attr dicts name ..._style out of a similar desire to make it more obvious what they are for. But then I considered it advantageous to name them exactly as their internal graphviz counterparts to stress that they are a 1:1 representation and make it easy for people to look up existing keywords in the graphviz docs.

[speth]

Just before I get started and not to over engineer this, but this is my first code review process: Is it preferable to address the requested changes as commits or as part of a rebase? Intuitively, I would try to incorporate trivial fixes like trailing whitespace into the original commits while creating new commits for stuff that might warrant a discussion/documentation.

We don't have a hard-and-fast rule on this. I tend to prefer squashing certain changes if it makes the history cleaner, and git commit --fixup=... makes this pretty easy most of the time. But if you find yourself having to resolve merge conflicts during a rebase, it might not be worth it.

And also to already address one of your suggestions @speth: I initially had the ..._attr dicts name ..._style out of a similar desire to make it more obvious what they are for. But then I considered it advantageous to name them exactly as their internal graphviz counterparts to stress that they are a 1:1 representation and make it easy for people to look up existing keywords in the graphviz docs.

It's definitely worth mentioning the corresponding graphviz names for these in the docstrings. Though if I understand this correctly, for the Cantera user, the API of the Python graphviz package isn't all that relevant. All they need to know is the options allowed by the underlying Graphviz "language", as defined by https://graphviz.org/doc/info/attrs.html. Is that correct?

[Naikless]

Any hints on how to get code coverage data reported from my fork?

The CI workflow exits with:

[2023-10-06T02:56:32.833Z] ['error'] There was an error running the uploader: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 404 - {'detail': ErrorDetail(string='Could not find a repository, try using repo upload token', code='not_found')}
[2023-10-06T02:56:32.834Z] ['verbose'] The error stack is: Error: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 404 - {'detail': ErrorDetail(string='Could not find a repository, try using repo upload token', code='not_found')}
    at main (/snapshot/repo/dist/src/index.js)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
[2023-10-06T02:56:32.834Z] ['verbose'] End of uploader: 1903 milliseconds
Error: Codecov: Failed to properly upload: The process '/home/runner/work/_actions/codecov/codecov-action/v3/dist/codecov' failed with exit code 255

My local machine is a Windows so I can't really get these information here either.

As far as I understand, I can create my own token for the repo with codecov, add it as a secret and use it in main.yaml. However, this of course doesn't make much sense for a fork. As a workaround I could try this in separate branch, but it seems like it is supposed to work without a token.

[speth]

It is supposed to work without a token, but Codecov seems to have occasional hiccups that require re-running the job. I just triggered a new run, which will hopefully work.

[Naikless]

Thanks. My workaround using an additional branch on my fork worked as well. The PR ist still WIP, I just wanted to check which spots I missed in my unit tests.

How do I have to invoke pytest to test the python modules locally? Simply calling pytest will either produce coverage of the test files themselves when run from cantera root dir or won't find any test when run from the local build.

[speth]

The recommended way to run the tests is through SCons, e.g. scons test-python if you just want to run the Python tests. If you want to run pytest directly, e.g. pytest test/python from the root directory, you need to make sure PYTHONPATH is set to the built copy of the module. If you're running with coverage enabled, I think this will inevitably count the test code itself. For Codecov, we just exclude it in the upload step (see the "coverage" job in .github/workflows/main.yml).

I find getting coverage reports to be finicky enough that I usually just rely on the GitHub Actions / Codecov build.

[Naikless]

It's definitely worth mentioning the corresponding graphviz names for these in the docstrings. Though if I understand this correctly, for the Cantera user, the API of the Python graphviz package isn't all that relevant. All they need to know is the options allowed by the underlying Graphviz "language", as defined by https://graphviz.org/doc/info/attrs.html. Is that correct?

That is correct. However, I thought about this once more: If each function only had a single Dict for these options, I would agree that naming it appearance or draw_style would be better suited. But all of them distinguish between node_attr, graph_attr and edge_attr dictionaries which flow into the graphviz API at different places and can contain the same keys with different values such as color. So the differentiation between "nodes" (reactors, reservoirs, most likely also reactor surfaces) and "edges" (mass flow controllers, walls) must be made regardless of the fact that the graphviz API is not accessed directly. In light of this, I believe sticking with the graphviz names for these dictionaries is the most straightforward choice.

But if anyone comes up with good names that fit the above criteria, I am more than happy to change my mind.

[Naikless]

Sorry, I didn't mean to close here but something went weird with the branch at my fork and I couldn't solve it without deleting it and pushing it again. Is it possible to restore the connection here or do I have to open a new PR?

[bryanwweber]

I think you just have to make a new PR, GitHub doesn't allow reconnecting to closed PRs once the branch is deleted as far as I know.

[Naikless]

Alright, I believe I have added all the functionality now that would make sense in my eyes. I also included ReactorSurface objects and wall movement.

The representation of the latter currently looks like this:

import cantera as ct
gas = ct.Solution('h2o2.yaml')
r = ct.Reactor(gas)
surf = ct.Interface('methane_pox_on_pt.yaml', 'Pt_surf')
s = ct.ReactorSurface(kin=surf, r=r)
s.draw()

r2 = ct.Reactor(gas)

w = ct.Wall(r, r2, velocity=1)
w.draw()

(Although it seems the little arrow at the wall sometimes isn't rendered.)

I also added the possibility to group reactors using a groupname attribute:

r3 = ct.Reactor(gas, groupname='group 1')

r.groupname = 'group 1'

net = ct.ReactorNet((r, r2, r3))
net.draw()

(Note that automatic reactor names increase regardless of the fact that reactors are repeatedly created)

Because a reactor's name defines its identity in the graphviz output, I had to ensure that each reactor is drawn with a unique name. However, until now there was nothing preventing people from giving the same name to multiple reactors and enforcing this now would be a potential breaking change. This added some additional complexity to how the functions interact with each other and the necessity to create a dictionary that saves all reactor names during an "external" function call. But I believe the solution works quite well.

So, from my point of view, the code is ready for another review.

What I haven't adressed yet:

• examples. I consider this to be the last step once everything else is sorted out.
• additional doc entries for the standalone functions. Since I would agree that access through the class methods is the main use case, I am not sure if adding redundantly documented standalone functions with the same functionality would be confusing. (Also, I have no idea how the doc build works yet. It's the only job still failing.)

[Naikless]

Thanks again for the review! I'll see to the suggested changes as soon as I can.

Quick question to potentially speed up my development: Is it possible to only run specific python tests using the scons command? Because some of them take a moment and in most cases I am working on it would suffice to just run the test-reactor module.

[speth]

Is it possible to only run specific python tests using the scons command?

Yes, you can run just the reactor tests with scons test-python-reactor.

It is also possible to run tests outside of SCons by running pytest directly, in which case you can filter tests using its -k option. However, this likely requires fiddling around with the PYTHONPATH and LD_LIBRARY_PATH (on Linux) environment variables.

[Naikless]

Thank you for your patience, implementing the changes again took me much longer than anticipated. Or I am just very bad at anticipating.

You can have another look if you like. Replacing the logic for renaming the objects if they share the same name with a simple assertion as well as splitting the branches for walls and flow controllers into their own functions hopefully made things a bit more concise.

I still get some error when compiling the docs. Is it still the formatting?

[Naikless]

Thanks a lot for merging this! Although I realize that the 3.0 release was quite recently, is there already a rough time frame for the next (pre-) release?