-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 an option for a functionalized cp for TemperaturePressureFunctionFP #29641
base: next
Are you sure you want to change the base?
Conversation
Use a different definition to compute cp from cv refs idaholab#21245
Job Documentation, step Docs: sync website on 3a96a6a wanted to post the following: View the site here This comment will be updated on new commits. |
refs idaholab#21245 Update regression test as cp expression changed
modules/fluid_properties/unit/include/TemperaturePressureFunctionFluidPropertiesTest.h
Outdated
Show resolved
Hide resolved
} | ||
|
||
Real | ||
TemperaturePressureFunctionFluidProperties::beta_from_p_T(Real pressure, Real temperature) const | ||
{ | ||
Real rho, drho_dp, drho_dT; | ||
rho_from_p_T(pressure, temperature, rho, drho_dp, drho_dT); | ||
return -drho_dT / rho; | ||
return -drho_dp / rho; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a bug fix (of my own code most likely)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was right before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://en.wikipedia.org/wiki/Compressibility
ugh. we use beta for what they call alpha
beta in our dosctring is "beta volumetric thermal expansion coefficient"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's pretty confusing. I see beta used for this coefficient sometimes, but alpha seems more common. At least the docstring appears correct and gives the units [1/K].
{ | ||
const Real tol = REL_TOL_CONSISTENCY; | ||
// Simpson integration only does 1e-5 | ||
const Real simpson_tol = 2e5 * tol; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is rough but yeah 3/8 simpson is there in accuracy. Maybe we should have a tunable method with a n-point quadrature instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here when we deviate from one of our typical tolerances, I think it's preferred to not express it as a product but just write directly
Job Coverage, step Generate coverage on 3a96a6a wanted to post the following: Framework coverage
Modules coverageFluid properties
Full coverage reportsReports
This comment will be updated on new commits. |
..._properties/doc/content/source/fluidproperties/TemperaturePressureFunctionFluidProperties.md
Show resolved
Hide resolved
..._properties/doc/content/source/fluidproperties/TemperaturePressureFunctionFluidProperties.md
Outdated
Show resolved
Hide resolved
modules/fluid_properties/src/fluidproperties/TemperaturePressureFunctionFluidProperties.C
Outdated
Show resolved
Hide resolved
modules/fluid_properties/src/fluidproperties/TemperaturePressureFunctionFluidProperties.C
Outdated
Show resolved
Hide resolved
3. / 8. * h * | ||
(cv_from_p_T(pressure, _T_ref) + 3 * cv_from_p_T(pressure, _T_ref + h) + | ||
3 * cv_from_p_T(pressure, _T_ref + 2 * h) + cv_from_p_T(pressure, temperature)); | ||
return _e_ref + integral; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you suggested this in another comment, but maybe a parameter with a numerical integral interval would be best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or at least use some kind of Simpson integral function defined outside of this class that takes a lambda or something.
..._properties/doc/content/source/fluidproperties/TemperaturePressureFunctionFluidProperties.md
Outdated
Show resolved
Hide resolved
..._properties/doc/content/source/fluidproperties/TemperaturePressureFunctionFluidProperties.md
Outdated
Show resolved
Hide resolved
..._properties/doc/content/source/fluidproperties/TemperaturePressureFunctionFluidProperties.md
Outdated
Show resolved
Hide resolved
..._properties/doc/content/source/fluidproperties/TemperaturePressureFunctionFluidProperties.md
Show resolved
Hide resolved
- better doco - fix beta (different notation than wikipedia) and cp-cv conversions Co-authored-by: joshuahansel <[email protected]>
this is going to help us study the solution to #21245
It should also work a lot better for gases.