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 vapor fraction as option for setting state in YAML files #784

Merged
merged 5 commits into from
Feb 12, 2020

Conversation

speth
Copy link
Member

@speth speth commented Jan 3, 2020

  • 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

Please fill in the issue number this pull request is fixing

Addressing a question from @ischoegl in on Cantera/cantera-website#91.

Changes proposed in this pull request

  • Allows (T, Q), (P, Q), and (T, P, Q) as state definitions for pure fluid phases. For the last one, requires the three variables to be self-consistent.
  • Fixes an issue that I ran into where leaving the pure-fluid-name out of the phase definition gave you a water object instead of throwing an exception.

@bryanwweber
Copy link
Member

Any reason not to cover the error messages and the vapor-fraction option with a test?

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.

Everything else looks good to me.

src/thermo/ThermoPhase.cpp Outdated Show resolved Hide resolved
@speth speth force-pushed the yaml-purefluid-state branch 2 times, most recently from 04674f9 to 66feab3 Compare January 10, 2020 16:04
@codecov
Copy link

codecov bot commented Jan 11, 2020

Codecov Report

Merging #784 into master will increase coverage by 0.02%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #784      +/-   ##
==========================================
+ Coverage   71.44%   71.46%   +0.02%     
==========================================
  Files         372      372              
  Lines       43952    43980      +28     
==========================================
+ Hits        31400    31430      +30     
+ Misses      12552    12550       -2
Impacted Files Coverage Δ
include/cantera/thermo/ThermoPhase.h 28.28% <ø> (ø) ⬆️
src/thermo/PureFluidPhase.cpp 59.72% <ø> (+0.08%) ⬆️
src/tpx/utils.cpp 94.59% <0%> (ø) ⬆️
test/thermo/thermoFromYaml.cpp 100% <100%> (ø) ⬆️
src/thermo/ThermoPhase.cpp 69.59% <95.65%> (+0.92%) ⬆️
src/base/AnyMap.cpp 87.9% <0%> (+0.4%) ⬆️

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 4eb375e...f7c33c6. Read the comment docs.

@bryanwweber
Copy link
Member

@speth I added one more test of the failure condition where the substance is unknown, to make sure that change isn't reverted. Feel free to drop it if you don't think it's necessary. This just seemed like the easiest way to add that change without a bunch of back and forth about what I meant 😄

@speth
Copy link
Member Author

speth commented Jan 14, 2020

The new phase definition you added breaks the test python:test_composite.TestModels.test_load_thermo_models, because it tries to instantiate every phase defined in thermo-models.yaml. If you really want to have a test for this, I would suggest putting the phase definition string in-line, rather than creating yet another input file just for this.

Ensure that unknown pure-fluid-name values throw an error.
@bryanwweber bryanwweber merged commit 451fa14 into Cantera:master Feb 12, 2020
speth added a commit to speth/cantera that referenced this pull request Feb 12, 2020
speth added a commit to speth/cantera that referenced this pull request Feb 12, 2020
ischoegl pushed a commit to ischoegl/cantera that referenced this pull request Feb 12, 2020
speth added a commit that referenced this pull request Feb 13, 2020
srikanthallu pushed a commit to srikanthallu/cantera that referenced this pull request Sep 17, 2020
@speth speth deleted the yaml-purefluid-state branch March 20, 2023 01:23
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