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

Linear kepsilon #29847

Open
wants to merge 13 commits into
base: next
Choose a base branch
from
Open

Linear kepsilon #29847

wants to merge 13 commits into from

Conversation

tanoret
Copy link
Contributor

@tanoret tanoret commented Feb 9, 2025

Closes #29846

@tanoret
Copy link
Contributor Author

tanoret commented Feb 9, 2025

@grmnptr @GiudGiud - could you please do the code review?
@freiler - could you please do the technical review? I normally run the tests in the flow test bed and was getting the same results than the k-epsilon with the nonlinear assembly

Thanks!!

@moosebuild
Copy link
Contributor

Job Precheck, step Clang format on b590f08 wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/docs/PRs/29847/clang_format/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format 5fce0ac136837e85742ff9c1539c66314ba1f773

Copy link
Member

@lindsayad lindsayad left a comment

Choose a reason for hiding this comment

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

Need some communication about why we are changing the framework objects. Seems to be to allow BCs on aux variables?

@@ -49,7 +49,7 @@ LinearFVBoundaryCondition::LinearFVBoundaryCondition(const InputParameters & par
MooseVariableInterface(this,
false,
"variable",
Moose::VarKindType::VAR_SOLVER,
Moose::VarKindType::VAR_ANY,
Copy link
Member

Choose a reason for hiding this comment

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

why this change?

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 allowing BCs on aux variables. Peter will do a separate PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, this is in this PR:
#29532

Comment on lines -7423 to -7431
try
{
computeSystems(EXEC_NONLINEAR);
}
catch (MooseException & e)
{
_console << "\nA MooseException was raised during Auxiliary variable computation.\n"
<< "The next solve will fail, the timestep will be reduced, and we will try again.\n"
<< std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

we definitely want try-catch

@moosebuild
Copy link
Contributor

Job Documentation, step Docs: sync website on 57a1783 wanted to post the following:

View the site here

This comment will be updated on new commits.

_rhs_non_time_tag(-1),
// We add this vector tag so that objects acting on the aux system inheriting
// from the tagging interface can still be used without any nonlinear systems
_rhs_non_time_tag(_fe_problem.addVectorTag("NONTIME")),
Copy link
Contributor

Choose a reason for hiding this comment

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

there's a cost to this I think ? Like in memory. Griffin people wont be happy

@@ -91,6 +94,10 @@ LinearSystem::LinearSystem(FEProblemBase & fe_problem, const std::string & name)
// We create a tag for the right hand side, the vector is already in the libmesh system
_rhs_tag = _fe_problem.addVectorTag("RHS");

// We add other wector tags so that objects acting on the aux system inheriting
// from the tagging interface can still be used without any nonlinear systems
_rhs_non_time_tag = _fe_problem.addVectorTag("NONTIME");
Copy link
Contributor

Choose a reason for hiding this comment

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

are we doing this operation twice? it s right above

Comment on lines +121 to +142
for (THREAD_ID tid = 0; tid < libMesh::n_threads(); tid++)
{
std::vector<LinearFVElementalKernel *> fv_elemental_kernels;
_fe_problem.theWarehouse()
.query()
.template condition<AttribSystem>("LinearFVElementalKernel")
.template condition<AttribThread>(tid)
.queryInto(fv_elemental_kernels);

for (auto * fv_kernel : fv_elemental_kernels)
fv_kernel->initialSetup();

std::vector<LinearFVFluxKernel *> fv_flux_kernels;
_fe_problem.theWarehouse()
.query()
.template condition<AttribSystem>("LinearFVFluxKernel")
.template condition<AttribThread>(tid)
.queryInto(fv_flux_kernels);

for (auto * fv_kernel : fv_flux_kernels)
fv_kernel->initialSetup();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use LinearFVKernel for the query instead?
initialSetup exists for this class, since it comes from the "LinearSystemContributionObject " base

@@ -0,0 +1,113 @@
# LinearFVTKEDSourceSink

This kernel implements a version of [`INSFVTKEDSourceSink`](INSFVTKEDSourceSink.md) for a linear kernel.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This kernel implements a version of [`INSFVTKEDSourceSink`](INSFVTKEDSourceSink.md) for a linear kernel.
This kernel implements a version of [`INSFVTKEDSourceSink`](INSFVTKEDSourceSink.md) for the linear finite volume discretization of the $k-\epsilon$ turbulence equations.

same for all other doco files

# LinearFVTKEDSourceSink

This kernel implements a version of [`INSFVTKEDSourceSink`](INSFVTKEDSourceSink.md) for a linear kernel.
The documentation is repeated down here for completeness.
Copy link
Contributor

Choose a reason for hiding this comment

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

use a file include instead? Create a new folder above source/ to host incomplete doco files that get included.

This will reduce maintenance cost

Comment on lines +195 to +199
// _console << "position: " << _current_elem_info->elem()->vertex_average() << std::endl;
// _console << "y_plus: " << y_plus << std::endl;
// _console << "TKE: " << TKE << std::endl;
// _console << "distance_vec[i]: " << distance_vec[i] << std::endl;
// _console << "destruction: " << destruction << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// _console << "position: " << _current_elem_info->elem()->vertex_average() << std::endl;
// _console << "y_plus: " << y_plus << std::endl;
// _console << "TKE: " << TKE << std::endl;
// _console << "distance_vec[i]: " << distance_vec[i] << std::endl;
// _console << "destruction: " << destruction << std::endl;

Comment on lines +206 to +207
// Do nothing
// return 0.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Do nothing
// return 0.0;

params.addParam<MooseEnum>("wall_treatment",
wall_treatment,
"The method used for computing the wall functions "
"'eq_newton', 'eq_incremental', 'eq_linearized', 'neq'");
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems only one method is implemented below

are the other s just the same for this kernel?

// Assembly variables
Real destruction = 0.0;
Real tot_weight = 0.0;
std::vector<Real> y_plus_vec, velocity_grad_norm_vec;
Copy link
Contributor

Choose a reason for hiding this comment

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

you should reserve a size for these two vectors for performance purposes rather than allocating dynamically later on


// If we are on an internal face, we populate the four entries in the system matrix
// which touch the face
if (_current_face_type == FaceInfo::VarFaceNeighbors::BOTH)
Copy link
Contributor

Choose a reason for hiding this comment

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

let's discuss this tomorrow. this is a ton code duplication for something that block restriction should achieve naturally?

Copy link
Contributor

@GiudGiud GiudGiud Feb 11, 2025

Choose a reason for hiding this comment

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

if you dont think so, then let's create "addMatrixContrib/ETCInternal in the advection class.

then in the base class (regular advection) you do: addMatrixContrib() { addMatrixContribInternal() }

and in the derived class, you do

if (on boundary)
  code code
else
 addMatrixContribInternal();

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.

Add k-epsilon model for new linear kernels
5 participants