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

Ion gas transport fix #568

Merged
merged 3 commits into from Nov 7, 2018
Merged

Conversation

BangShiuh
Copy link
Contributor

This PR is for some issues found in IonGasTransport.
Changes proposed in this pull request:

  • Fix getMobilities. It didn't update temperature and other parameters before the calculation.
  • Add a test to prevent this happen again.
  • Not sure why I can't add mobilities to cython. I tried some and put them in comment.

@codecov
Copy link

codecov bot commented Oct 19, 2018

Codecov Report

Merging #568 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #568      +/-   ##
==========================================
+ Coverage   64.92%   64.92%   +<.01%     
==========================================
  Files         386      386              
  Lines       40800    40804       +4     
==========================================
+ Hits        26489    26493       +4     
  Misses      14311    14311
Impacted Files Coverage Δ
include/cantera/cython/wrappers.h 87.03% <ø> (ø) ⬆️
src/transport/IonGasTransport.cpp 84.82% <100%> (+0.27%) ⬆️

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 87b042a...8138fbe. Read the comment docs.

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.

The spelling error noted below is presumably all that is preventing the Python method from working. After fixing that, I'm assuming you can get the new tests in working order.

@@ -858,6 +858,7 @@ cdef extern from "cantera/cython/wrappers.h":
cdef void tran_getMixDiffCoeffsMole(CxxTransport*, double*) except +translate_exception
cdef void tran_getThermalDiffCoeffs(CxxTransport*, double*) except +translate_exception
cdef void tran_getSpeciesViscosities(CxxTransport*, double*) except +translate_exception
# cdef void tran_getMobilities(CxxTransport*, double*) except +translate_excepttion
Copy link
Member

Choose a reason for hiding this comment

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

translate_exception is misspelled.

@BangShiuh
Copy link
Contributor Author

Thank you. It is hard to spot this kind of typo.

@speth
Copy link
Member

speth commented Nov 7, 2018

No problem, and thanks for the updates. I made a few small changes to the tests -- as written, they relied on the tests executing in the order written, which is not guaranteed.

@speth speth merged commit acd75e2 into Cantera:master Nov 7, 2018
@BangShiuh BangShiuh deleted the IonGasTransport_fix branch December 2, 2018 17:29
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

2 participants