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 physics for fully coupled k-epsilon #27614

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

Conversation

GiudGiud
Copy link
Contributor

@GiudGiud GiudGiud commented May 13, 2024

TODO:

  • documentation
  • add energy and scalar volumetric terms
  • add energy wall function terms
  • add scalar wall function terms
  • add cross-parameter checking between turbulence and other physics
  • expand testing reasonably. There are already 14 regression tests for turbulence trying out each wall function

It might take a bit to finish this. There's a lot, and a lot of options that don't work together.
I ll get back to Components

@moosebuild
Copy link
Contributor

moosebuild commented May 14, 2024

Job Documentation on 54e2eb3 wanted to post the following:

View the site here

This comment will be updated on new commits.

@moosebuild
Copy link
Contributor

moosebuild commented Jul 5, 2024

Job Coverage on 2c3e6e5 wanted to post the following:

Framework coverage

634916 #27614 2c3e6e
Total Total +/- New
Rate 85.06% 85.06% -0.00% 0.00%
Hits 104535 104532 -3 0
Misses 18359 18362 +3 1

Diff coverage report

Full coverage report

Modules coverage

Navier stokes

634916 #27614 2c3e6e
Total Total +/- New
Rate 84.59% 83.50% -1.09% 33.33%
Hits 16205 16301 +96 141
Misses 2952 3221 +269 282

Diff coverage report

Full coverage report

Full coverage reports

Reports

Warnings

  • framework new line coverage rate 0.00% is less than the suggested 90.0%
  • navier_stokes new line coverage rate 33.33% is less than the suggested 90.0%

This comment will be updated on new commits.

Copy link
Contributor

@tanoret tanoret left a comment

Choose a reason for hiding this comment

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

Some very minor comments in case they help. They are mostly oriented at not deteriorating the performance of the linear solver. Nice work!

Comment on lines 164 to 167
destruction += destruction_visc;
destruction += destruction_visc + 0 * destruction_log;
else
destruction += destruction_log;
destruction += destruction_log + 0 * destruction_visc;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please note that this is adding unneessary computational burden to the SIMPLE-like solvers. Please add a switch to identify if we are trying to solve these equations with a simple solve. If we are, please don't do the multiplication by 0.

Comment on lines +117 to 116
mooseAssert(distance_vec.size(), "Should have found a distance vector");
mooseAssert(distance_vec.size() == face_info_vec.size(),
"Should be as many distance vectors as face info vectors");

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these cehck needed at this point? It seems to be adding computational burden every time that the kernel is called

Copy link
Contributor Author

Choose a reason for hiding this comment

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

asserts are removed from opt mode. It only affects devel builds, in a very minor way. They are a good practice


for (unsigned int i = 0; i < distance_vec.size(); i++)
{
const auto distance = distance_vec[i];
mooseAssert(distance > 0, "Should be at a non-zero distance");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this check needed at this point? It seems to justbe adding computational burden every time that we call this kernel. Please advise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same here. It does not hurt

@@ -148,7 +155,7 @@ INSFVTKEDSourceSink::computeQpResidual()
destruction += 2.0 * TKE * wall_mut / Utility::pow<2>(distance_vec[i]) / tot_weight;
}
else
destruction += std::pow(_C_mu, 0.75) * rho * std::pow(TKE, 1.5) /
destruction += std::pow(_C_mu, 0.75) * rho * std::pow(std::max(ADReal(0), TKE), 1.5) /
Copy link
Contributor

Choose a reason for hiding this comment

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

TKE should be always positive by the conservation properties of the k epsilon mode. This capping may be unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the nonlinear solver was making it negative. It's unfortunate but it happens with Newton methods

@@ -24,7 +24,8 @@ kEpsilonViscosityAux::validParams()
params.addRequiredParam<MooseFunctorName>("u", "The velocity in the x direction.");
params.addParam<MooseFunctorName>("v", "The velocity in the y direction.");
params.addParam<MooseFunctorName>("w", "The velocity in the z direction.");
params.addRequiredParam<MooseFunctorName>(NS::TKE, "Coupled turbulent kinetic energy.");
params.addRequiredParam<MooseFunctorName>("k", "Coupled turbulent kinetic energy.");
Copy link
Contributor

Choose a reason for hiding this comment

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

To keep it consistent, please only use variable names in the NS namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am deprecating the old name "k" and replacing it with the new NS::TKE (= tke)

@@ -20,7 +20,8 @@ INSFVMixingLengthTKEDBC::validParams()
params.addClassDescription("Adds inlet boundary condition for the turbulent kinetic energy "
"dissipation rate based on characteristic length.");
params.addParam<MooseFunctorName>("C_mu", 0.09, "Coupled turbulent kinetic energy closure.");
params.addRequiredParam<MooseFunctorName>(NS::TKE, "The turbulent kinetic energy.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. Please try to only use variable names in the NS namespace

@@ -26,7 +26,8 @@ INSFVTKEDWallFunctionBC::validParams()
params.addRequiredParam<MooseFunctorName>(NS::density, "Density");
params.addRequiredParam<MooseFunctorName>(NS::mu, "Dynamic viscosity.");
params.addRequiredParam<MooseFunctorName>(NS::mu_t, "The turbulent viscosity.");
params.addRequiredParam<MooseFunctorName>(NS::TKE, "The turbulent kinetic energy.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please try to only use variable names in the NS namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see first comment, this is using NS but NS got changed so we have to code "k" as the deprecated parameter

@@ -82,14 +84,16 @@ INSFVTKEDWallFunctionBC::boundaryValue(const FaceInfo & fi) const
if (y_plus <= 5.0) // sub-laminar layer
{
const auto laminar_value = 2.0 * weight * TKE * mu / std::pow(dist, 2);
return laminar_value.value();
// Additional zero term to make sure new derivatives are not introduced as y_plus changes
return laminar_value + 0 * _mu_t(makeElemArg(&_current_elem), state);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is adding unnecessary computational burden to the linear solver. Please add a switch between linear and nonlinear if this is needed for the nonlinear solver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

those are mostly removed

}
else if (y_plus >= 30.0) // log-layer
{
const auto turbulent_value = weight * _C_mu(makeElemArg(&_current_elem), state) *
std::pow(std::abs(TKE), 1.5) /
(_mu_t(makeElemArg(&_current_elem), state) * dist);
return turbulent_value.value();
// Additional zero term to make sure new derivatives are not introduced as y_plus changes
return turbulent_value + 0 * mu;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for this one. This is adding unnecessary computational burden to the linear solver. Please add a switch between linear and nonlinear if this is needed for the nonlinear solver

@GiudGiud GiudGiud force-pushed the PR_turb_keps branch 3 times, most recently from 8f806c8 to 022d722 Compare July 20, 2024 00:20
@GiudGiud GiudGiud force-pushed the PR_turb_keps branch 3 times, most recently from 612ff4e to 6905537 Compare July 20, 2024 00:28
- dont shed mu_t derivatives
- use NS::computeSpeed to survive negative init
- add 0 x other branch for y_plus selections of code (y_plus can change during the nonlinear solve)
better than turbulence as it simplifies boundary-checking logic
…ore cleanly

and be able to check walls between heat and flow equations for lid-driven cavities
- compare to full results as the 'short' tests do not converge the solution
We still expect some minor differences between segregated and fully coupled (relaxation coeffs dependence for example)
Add support for C_pl coeff in tubrulence physics
…to linearized)

Fixup kt aux-variable creation in turb physics
Add matching outputs to segregated inputs
Try bounds on k and epsilon
…d objects

Clarify lagged/not lagged terms a bit. Limit number of functor evaluations
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

3 participants