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

Add thermal conductivity to DustyGas in python. #988

Merged
merged 3 commits into from
Apr 19, 2021

Conversation

chinahg
Copy link
Member

@chinahg chinahg commented Mar 6, 2021

Changes proposed in this pull request

  • Thermal conductivity can now be calculated using the DustyGas class in python

If applicable, fill in the issue number this pull request is fixing

Fixes #

Checklist

  • There is a clear use-case for this code change
  • The commit message has a short title & references relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • The pull request is ready for review

@codecov
Copy link

codecov bot commented Mar 6, 2021

Codecov Report

Merging #988 (a9146f3) into main (2cd0619) will increase coverage by 1.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #988      +/-   ##
==========================================
+ Coverage   71.22%   72.40%   +1.18%     
==========================================
  Files         377      364      -13     
  Lines       46272    46433     +161     
==========================================
+ Hits        32955    33618     +663     
+ Misses      13317    12815     -502     
Impacted Files Coverage Δ
include/cantera/transport/DustyGasTransport.h 0.00% <ø> (-33.34%) ⬇️
...clude/cantera/transport/HighPressureGasTransport.h 0.00% <0.00%> (-33.34%) ⬇️
include/cantera/zeroD/flowControllers.h 61.76% <0.00%> (-24.45%) ⬇️
src/equil/vcs_solve.cpp 67.07% <0.00%> (-22.11%) ⬇️
include/cantera/thermo/VPStandardStateTP.h 50.00% <0.00%> (-21.43%) ⬇️
include/cantera/thermo/MixtureFugacityTP.h 0.00% <0.00%> (-20.00%) ⬇️
include/cantera/zeroD/FlowReactor.h 66.66% <0.00%> (-15.69%) ⬇️
src/thermo/IdealSolnGasVPSS.cpp 55.37% <0.00%> (-13.34%) ⬇️
include/cantera/thermo/GibbsExcessVPSSTP.h 14.28% <0.00%> (-10.72%) ⬇️
... and 323 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cd0619...a9146f3. Read the comment docs.

@speth speth requested a review from decaluwe March 12, 2021 19:10
Copy link
Member

@decaluwe decaluwe left a comment

Choose a reason for hiding this comment

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

Thanks, @chinahg. This looks great. Coding looks straightforward, and I pulled the code to my machine and verified that it works in Python.

Two questions, both regarding demonstration/testing:

  1. Can we add some code demonstrating that this is functional in the dusty_gas.py example?
  2. It looks like test coverage increases as a result of this change, but unless I'm mistaken, it doesn't look like this specific capability is tested. Can you add something to the dustyGasTransport test problem to test that this function works?

Great job!

@decaluwe
Copy link
Member

Thanks, @chinahg -- change to the example file looks simple but effective (both of those are good things, for the record 😉).

Now if we can get something testing this in the associated test problem, this will be all ready to merge. 🎉

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.

@decaluwe, there are no code changes to the C++ implementation here, so I don't think there's anything to test in the C++ test. I think the coverage changes are all spurious, and just an indication that this should be rebased onto the current main branch.

@chinahg, can you add a test to the Python test suite? You can add it to the TestDustyGas class in interfaces/cython/cantera/test/test_transport.py.

include/cantera/transport/DustyGasTransport.h Outdated Show resolved Hide resolved
interfaces/cython/cantera/transport.pyx Show resolved Hide resolved
@decaluwe
Copy link
Member

Good point, @speth -- thanks.

@decaluwe
Copy link
Member

Many thanks and excellent work, @chinahg!

@decaluwe decaluwe merged commit fe29628 into Cantera:main Apr 19, 2021
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.

3 participants