Skip to content

Add vapor fraction as option for setting state in YAML files#784

Merged
bryanwweber merged 5 commits intoCantera:masterfrom
speth:yaml-purefluid-state
Feb 12, 2020
Merged

Add vapor fraction as option for setting state in YAML files#784
bryanwweber merged 5 commits intoCantera:masterfrom
speth:yaml-purefluid-state

Conversation

@speth
Copy link
Copy Markdown
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.

@speth speth mentioned this pull request Jan 3, 2020
Merged
@bryanwweber
Copy link
Copy Markdown
Member

bryanwweber commented Jan 4, 2020

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

ischoegl
ischoegl reviewed Jan 8, 2020
View reviewed changes
Copy link
Copy Markdown
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.

@speth speth force-pushed the yaml-purefluid-state branch 2 times, most recently from 04674f9 to 66feab3 Compare January 10, 2020 16:04
@speth speth requested a review from bryanwweber January 10, 2020 18:38
@speth speth force-pushed the yaml-purefluid-state branch from 66feab3 to decf41c Compare January 11, 2020 17:36
@codecov
Copy link
Copy Markdown

codecov bot commented Jan 11, 2020 •
edited
Loading

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
Copy Markdown
Member

bryanwweber commented Jan 14, 2020

@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
Copy Markdown
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.

@bryanwweber
[Test] Add test of unknown pure fluid
f7c33c6 
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
Fix test failure resulting from interaction between PRs Cantera#784 and
7b70894 
Cantera#796
speth added a commit to speth/cantera that referenced this pull request Feb 12, 2020
@speth
Fix test failure resulting from interaction between PRs Cantera#784 and
6c116fc 
Cantera#796

Fixes Cantera#807
ischoegl pushed a commit to ischoegl/cantera that referenced this pull request Feb 12, 2020
@speth @ischoegl
Fix test failure resulting from interaction between PRs Cantera#784 and
faf2f29 
Cantera#796

Fixes Cantera#807
@speth speth mentioned this pull request Feb 12, 2020
Merged
4 tasks
speth added a commit that referenced this pull request Feb 13, 2020
@speth
Fix test failure resulting from interaction between PRs #784 and #796
436c2e7 
Fixes #807
srikanthallu pushed a commit to srikanthallu/cantera that referenced this pull request Sep 17, 2020
@speth @srikanthallu
Fix test failure resulting from interaction between PRs Cantera#784 and
1cffe91 
Cantera#796

Fixes Cantera#807
@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

Reviewers

@ischoegl ischoegl ischoegl left review comments

@bryanwweber bryanwweber bryanwweber approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@speth @bryanwweber @ischoegl