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 2ad20c4 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 Cantera/enhancements#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

• 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)
• unit tests address code coverage
• Style & formatting of contributed code follows contributing guidelines
• 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]

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 ⚠️

Additional details and impacted files

@@            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     

☔ View full report in Codecov by Sentry.
📢 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.

[speth]

Thanks, @Naikless, this is a really nice contribution, and addresses a feature request that I know has been made by many users.

To answer your questions:

• Using dicts for Reactor.node_attr and the like are a fine way of storing styling information. The only thing I might suggest changing is the name, since talking about "nodes" and "edges" really only makes sense if you understand that this is a graph in the mathematical sense. One alternative that comes to mind is appearance, which would work for both reactors and flow devices.
• The lazy import of graphviz looks good to me
• I think you can just omit the imports in __init__.py, leaving the member functions as the main API.
• I'd suggest adding the tests to test_reactor.py, but I'm guessing you'll want to configure some different reactor networks than the ones that it uses, so you probably want to create a separate TestDrawNetwork or similar class.

Some additional comments:

• A way of including ReactorSurface objects would be useful
• Is there a way to indicate wall motion? I guess at some level, this is a transfer of volume from one reactor to another, though it might look a little funny.
• It would be good add some example usage. I was going to suggest adding this to some of the existing examples, but most of them have almost trivial network structures. I guess mix1.py at least has a reactor with two inlets and one outlet. ic_engine.py has two inlets as well, but it's less clear what the right thing to show would be, given the transients that it's simulating.
• The fact that ReactorNet.draw() plays nicely with Jupyter is great.
• I think you need to add a couple of entries in zerodim.rst so the standalone functions will be included in the API docs.

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

[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]

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?

[speth]

Thanks for the updates, @Naikless. I made a few fixes to the formatting of the documentation, so I think this is good to go assuming the CI completes without error.

[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?