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 an option for a functionalized cp for TemperaturePressureFunctionFP #29641

Draft
wants to merge 3 commits into
base: next
Choose a base branch
from

Conversation

GiudGiud
Copy link
Contributor

@GiudGiud GiudGiud commented Jan 4, 2025

this is going to help us study the solution to #21245
It should also work a lot better for gases.

Use a different definition to compute cp from cv
refs idaholab#21245
@GiudGiud GiudGiud self-assigned this Jan 4, 2025
@moosebuild
Copy link
Contributor

moosebuild commented Jan 4, 2025

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
@GiudGiud GiudGiud marked this pull request as ready for review January 4, 2025 19:55
@GiudGiud GiudGiud requested a review from joshuahansel as a code owner January 4, 2025 19:55
}

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;
Copy link
Contributor Author

@GiudGiud GiudGiud Jan 4, 2025

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)

Copy link
Contributor

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.

Copy link
Contributor Author

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"

Copy link
Contributor

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;
Copy link
Contributor Author

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.

Copy link
Contributor

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

@moosebuild
Copy link
Contributor

moosebuild commented Jan 4, 2025

Job Coverage, step Generate coverage on 3a96a6a wanted to post the following:

Framework coverage

1e1768 #29641 3a96a6
Total Total +/- New
Rate 85.25% 85.25% -0.00% -
Hits 108011 108010 -1 0
Misses 18681 18682 +1 0

Diff coverage report

Full coverage report

Modules coverage

Fluid properties

1e1768 #29641 3a96a6
Total Total +/- New
Rate 85.19% 85.29% +0.10% 93.96%
Hits 7777 7873 +96 140
Misses 1352 1358 +6 9

Diff coverage report

Full coverage report

Full coverage reports

Reports

This comment will be updated on new commits.

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;
Copy link
Contributor

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.

Copy link
Contributor

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.

- better doco
- fix beta (different notation than wikipedia) and cp-cv conversions

Co-authored-by: joshuahansel <[email protected]>
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