-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
Implementation of MoleReactor and ConstantPressureMoleReactor #1363
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1363 +/- ##
==========================================
+ Coverage 70.99% 71.04% +0.05%
==========================================
Files 357 359 +2
Lines 51561 51707 +146
Branches 17267 17327 +60
==========================================
+ Hits 36605 36735 +130
+ Misses 12643 12634 -9
- Partials 2313 2338 +25
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
9b74968
to
1cd3b33
Compare
1ddef70
to
b7d303d
Compare
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, @anthony-walker, it's good to see the set of reactor types more fleshed out.
Besides the few comments below, one thing I think you should do is add the Extensible...Reactor
wrappers for all of the mole-based reactor classes so they can be used as the basis for user-defined reactors as well.
@speth Thank you for the review. I have made all of the requested changes. Aside from the following because
I was considering potentially adding some tests for the |
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.
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 for the updates, @anthony-walker. I agree with you on leaving the use of m_mass
in getState
alone for now.
I think the only remaining issues are two small documentation updates.
This commit implements MoleReactor and ConstPressureMoleReactor classes, it adds the appropriate python interfaces for them, it also adds a test comparing surface chemistry of mole reactors to traditional reactors. test_component_names had a bug because the network was not initialized so self.net.n_vars was 0 and the loop was never entered. Adding a line to initialize the network reveal that the test failed for IdealGasMoleReactor so the appropriate lines were added to fix it. Add wrappers for Extensible...MoleReactor, update docs, comment fixes.
Co-authored-by: Ingmar Schoegl <ischoegl@lsu.edu>
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, @anthony-walker. This looks good to me. I'll merge once the tests all pass.
Changes proposed in this pull request
test_reactor.py::test_component_names
Checklist
scons build
&scons test
) and unit tests address code coverage