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

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

Thank you! This is very cool

do a cleanup commit

A cleanup commit is fine 😁

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.

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.

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?

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.

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.

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.

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.

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.

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.

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

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?

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?