Skip to content
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

Merged
merged 3 commits into from Sep 9, 2022

Conversation

anthony-walker
Copy link
Contributor

@anthony-walker anthony-walker commented Aug 11, 2022

Changes proposed in this pull request

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) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

@codecov
Copy link

codecov bot commented Aug 11, 2022

Codecov Report

Merging #1363 (bcd8124) into main (e3080ed) will increase coverage by 0.05%.
The diff coverage is 75.63%.

@@            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     
Impacted Files Coverage Δ
...e/cantera/zeroD/IdealGasConstPressureMoleReactor.h 66.66% <ø> (-33.34%) ⬇️
include/cantera/zeroD/MoleReactor.h 66.66% <ø> (+55.55%) ⬆️
src/zeroD/ConstPressureMoleReactor.cpp 56.71% <56.71%> (ø)
include/cantera/zeroD/ConstPressureMoleReactor.h 66.66% <66.66%> (ø)
src/zeroD/ReactorFactory.cpp 86.66% <66.66%> (-13.34%) ⬇️
src/zeroD/MoleReactor.cpp 90.66% <86.27%> (+40.66%) ⬆️
interfaces/cython/cantera/reactor.pyx 89.41% <100.00%> (+0.17%) ⬆️
src/zeroD/IdealGasConstPressureMoleReactor.cpp 73.60% <100.00%> (-1.59%) ⬇️
src/zeroD/IdealGasMoleReactor.cpp 92.85% <100.00%> (+8.02%) ⬆️
... and 8 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@anthony-walker anthony-walker marked this pull request as draft August 13, 2022 16:57
@anthony-walker anthony-walker marked this pull request as ready for review August 13, 2022 16:58
@anthony-walker anthony-walker changed the title Implementation of MoleReactor and Constant Pressure Mole Reactor Implementation of MoleReactor and Constant PressureMoleReactor Aug 16, 2022
@anthony-walker anthony-walker changed the title Implementation of MoleReactor and Constant PressureMoleReactor Implementation of MoleReactor and ConstantPressureMoleReactor Aug 16, 2022
Copy link
Member

@speth speth left a 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.

src/zeroD/ConstPressureMoleReactor.cpp Outdated Show resolved Hide resolved
src/zeroD/ConstPressureMoleReactor.cpp Outdated Show resolved Hide resolved
test/python/test_reactor.py Outdated Show resolved Hide resolved
test/python/test_reactor.py Outdated Show resolved Hide resolved
@anthony-walker
Copy link
Contributor Author

anthony-walker commented Aug 27, 2022

@speth Thank you for the review. I have made all of the requested changes. Aside from the following because m_mass is used in the getMoles function. I have added a comment to reflect this as well and avoid confusion.

The value of m_mass is not used in this case, so I don't think there's a need to set it in this method.

I was considering potentially adding some tests for the Extensible...MoleReactor classes as well if you think necessary. Thoughts?

Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as a minor comment, as of 3.0 we are trying to keep track of newly introduced features with doxygen and Sphinx tags (should have pointed that out in #1003 and #1377).

interfaces/cython/cantera/reactor.pyx Outdated Show resolved Hide resolved
Copy link
Member

@speth speth left a 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.

include/cantera/zeroD/ConstPressureMoleReactor.h Outdated Show resolved Hide resolved
interfaces/cython/cantera/reactor.pyx Show resolved Hide resolved
anthony-walker and others added 3 commits September 9, 2022 13:17
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>
@anthony-walker
Copy link
Contributor Author

@speth and @ischoegl, thank you for the review. Assuming all checks pass, I have addressed all of your comments and concerns.

Copy link
Member

@speth speth left a 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.

@speth speth merged commit c91e9eb into Cantera:main Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants