-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Cantera 2.5] Update valve/etc. in science section #104
Conversation
@ischoegl You can rebase this onto |
@ischoegl What's the status of this PR? Thanks! |
@bryanwweber ... will have to look over recent changes on the MFC. Will certainly tackle this as soon as beta is reached. |
Thanks @ischoegl I agree waiting for beta makes sense. |
1a5d7b1
to
8eef115
Compare
34edb69
to
3f9cff6
Compare
Descriptions of mass flow devices (`Valve`, `MassFlowController` and `PressureController`) are updated due to changes in Cantera/cantera#667.
3f9cff6
to
b9189d5
Compare
@bryanwweber ... now that 2.5 is on the horizon, I finally updated the docs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for updating this! A few formatting comments here. I think it would make more sense to keep the symbols here consistent with the docstrings, keeping K_v
instead of c
.
One problem is that the :py:meth:
role isn't defined, but I can't recall right now how to link to Python documentation methods... I think it's :py:func:
with the fully classified name (i.e., MassFlowController.set_time_function
). You can shorten the name to just the method by putting a tilde before the class name.
That said, there doesn't actually appear to be any documentation for the various attributes and methods of these classes, see: https://cantera.org/documentation/dev/sphinx/html/cython/zerodim.html#massflowcontroller I'm not sure what happened to those.
@ischoegl Looking at the dev documentation here: https://cantera.org/documentation/dev/sphinx/html/cython/zerodim.html#massflowcontroller I still see |
@bryanwweber ... Yes ... I just realized that I had looked at C++ as well (which is why I deleted my earlier declaration to be done). I agree that Python is the more likely avenue, and am mostly done with updating the docs accordingly.
|
264a891
to
97786f9
Compare
OK. I force-pushed - Regarding the differences between C++ and Python, all coefficients are now defined for the parent object using the same private member variable |
97786f9
to
79ee18e
Compare
@bryanwweber ... finally located the reference I was wondering about: https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html#cross-referencing-python-objects (which curiously mentions Updated accordingly, so I believe this should be done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ischoegl!
FYI @speth this will require a few more changes to the docs from Cantera/cantera, I'm working on a PR. We should wait to merge this until after 2.5.1. |
ok. I am aware of remaining activity on the website. Hmm ... I may not have gotten the correct impression based on the comment. If I understand, it is scheduled to be merged, but requires some fixes later on ... |
@ischoegl thanks again for updating this. As you can probably see, we've tagged 2.5.1 already which incorporates the fixes to the docs. The problem wasn't with this code, but with the fact that Sphinx doesn't include inherited members by default, so the |
@bryanwweber .... Ok, thanks! I saw your fix earlier today. Just didn’t quite know about the context earlier. All the best for the rollout! |
Addresses @speth's comment in Cantera/cantera#667
Per @bryanwweber's comment in #88, the update should be parked as a PR until the release of 2.5.