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

Support for two term FV gradient expansion with triangle mesh #16822

Closed
GiudGiud opened this issue Jan 28, 2021 · 7 comments
Closed

Support for two term FV gradient expansion with triangle mesh #16822

GiudGiud opened this issue Jan 28, 2021 · 7 comments
Labels
C: Modules P: normal A defect affecting operation with a low possibility of significantly affects. T: defect An anomaly, which is anything that deviates from expectations.

Comments

@GiudGiud
Copy link
Contributor

GiudGiud commented Jan 28, 2021

Bug Description

In INSFV simulations:
When using triangle mesh with a mesh cell in a corner, which has both a wall and a flow boundary condition, the matrix (the one solved to obtain the face gradients, the centroid gradient and the face values of velocity) is reported as singular and the simulation diverges.
In general FV simulations, it's likely the issue will appear with future ZeroNormalGradient boundary conditions

Steps to Reproduce

Any mesh with a triangle cell that covers an entire corner. It's likely though unconfirmed that splitting the corner with two mesh cells is sufficient to remove the problem. Indeed if the corner is split as is done automatically by gmsh (see below) then there is no issue. The diverging channel tests in INSFV use the below mesh with two term boundary expansion.
Screenshot from 2021-04-29 17-50-53

It's likely though unconfirmed the issue can also be obtained on quad meshes with different combinations of boundary conditions (3 boundaries for a cell for example). It has been confirmed that if three faces of a (orthogonal) quad are on a boundary, then singularity occurs

Impact

This forces the use of quad meshes in INSFV simulations.

@GiudGiud GiudGiud added the T: defect An anomaly, which is anything that deviates from expectations. label Jan 28, 2021
@GiudGiud GiudGiud added this to To do in FY 21 NEAMS TH Pronghorn via automation Jan 28, 2021
@lindsayad lindsayad added C: Modules P: normal A defect affecting operation with a low possibility of significantly affects. labels Feb 2, 2021
@lindsayad
Copy link
Member

Yea I don't think there's much I can do about this. I worked this out by hand for the tri test case in the postprocessors directory, and the matrix is just straight up singular. I could resolve it by projecting dCb onto the surface normal vector ... that would guarantee to remove the singularity. Essentially then you are just extrapolating the boundary face value with the part of the gradient that is in the face normal direction

@lindsayad
Copy link
Member

lindsayad commented Apr 29, 2021

This patch allows me to run the postprocessor tri test case with two term boundary expansion. If you think this is a satisfactory enough solution, then I will submit a PR

diff --git a/framework/src/variables/MooseVariableFV.C b/framework/src/variables/MooseVariableFV.C
index 11bee5484c..4c29825a62 100644
--- a/framework/src/variables/MooseVariableFV.C
+++ b/framework/src/variables/MooseVariableFV.C
@@ -782,9 +782,15 @@ MooseVariableFV<OutputType>::adGradSln(const Elem * const elem) const
         ebf_faces.push_back(fi);
 
         // eqn. 2
-        ebf_grad_coeffs.push_back(-1. * (elem_has_info
-                                             ? (fi->faceCentroid() - fi->elemCentroid())
-                                             : (fi->faceCentroid() - fi->neighborCentroid())));
+        const auto surface_normal = surface_vector / surface_vector.norm();
+        const auto dCb = elem_has_info ? (fi->faceCentroid() - fi->elemCentroid())
+                                       : (fi->faceCentroid() - fi->neighborCentroid());
+        // We project dCb onto the surface_normal direction in order to prevent singularities when
+        // we have multiple extrapolated boundary faces per element
+        mooseAssert(dCb * surface_normal >= 0,
+                    "I think these things should be in the same direction...");
+        const auto dCbn = (dCb * surface_normal) * surface_normal;
+        ebf_grad_coeffs.push_back(-1. * dCbn);
         ebf_b.push_back(&elem_value);
 
         // eqn. 1

@GiudGiud
Copy link
Contributor Author

GiudGiud commented Apr 29, 2021

I like that, but I want to know more

Does that break all the other tests?

Do you think it's the same scenario for the extrapolation with tetrahedra?

Is it only when two of the three faces are extrapolated? Or already with a single extrapolated face?

I can't see anything in Moukalled about a special treatment for those meshes. Can you think of another reference that might have that?

@lindsayad
Copy link
Member

lindsayad commented Apr 29, 2021

I like that, but I want to know more

🤣

Does that break all the other tests?

About to find out

Do you think it's the same scenario for the extrapolation with tetrahedra?

Is it only when two of the three faces are extrapolated? Or already with a single extrapolated face?

It requires multiple extrapolated faces and it requires that for at least one of the extrapolated faces that (dCf * n) n != dCf

I can't see anything in Moukalled about a special treatment for those meshes. Can you think of another reference that might have that?

Moukalled doesn't seem to go into detail on a lot of the finer points. For instance skew corrections get half a page and there is no mention of solving for a gradient with Green-Gauss when you have extrapolated boundary faces. This article goes into quite a bit of detail on gradient computation but seems to imply that Green-Gauss is second order at internal faces and first order at boundary faces ... which is only true if you're assuming you're going to use the internal cell value as the boundary face value.

@lindsayad
Copy link
Member

Or perhaps they're referring to the accuracy of the gradient itself? In that case I'd be shocked if Green-Gauss yields a second-order gradient. Usually if an error norm for the solution is order N, then a similar error norm for the gradient is order N - 1.

lindsayad added a commit to lindsayad/moose that referenced this issue Apr 29, 2021
This fixes idaholab#16822 although perhaps not 100% satisfactorily. This
solution will maintain the second order solution accuracy if we are on
an orthgonal grid but likely will not on a non-orthogonal grid since we
are only using the gradient in the surface normal direction to to help
extrapolate the boundary face value
lindsayad added a commit to lindsayad/moose that referenced this issue Apr 29, 2021
This fixes idaholab#16822 although perhaps not 100% satisfactorily. This
solution will maintain the second order solution accuracy if we are on
an orthgonal grid but likely will not on a non-orthogonal grid since we
are only using the gradient in the surface normal direction to to help
extrapolate the boundary face value
lindsayad added a commit to lindsayad/moose that referenced this issue Apr 30, 2021
This fixes idaholab#16822 although perhaps not 100% satisfactorily. If a user
knows they only have cells with a maximum of one extrapolated boundary
face or if they know that if the cell has multiple extrapolated boundary
faces that it is orthogonal (e.g. dCf is parallel to Sf), then they can
safely set `two_term_boundary_expansion = true` in their input. But in
the interest of allowing more things to just work out of the box (like
any old tri or tet mesh), I think it makes sense to make the one term
expansion the default. I prefer doing this over something like
projecting dCf onto the surface normal because this would cost a user
accuracy in the case where they have intelligently designed their mesh
such that there is only a maximum of one extrapolated boundary face per
cell (gmsh meshes for example avoid this).
@lindsayad
Copy link
Member

I ended up deciding to go the route of just using one-term boundary expansion by default for INSFV. You can see my explanation in #17716. If we keep the current title of this issue, then I don't think #17716 closes it, but if we amend the title to "cannot run INSFV with many triangle/tet meshes" then #17716 can be considered a fix. Up to you

@GiudGiud
Copy link
Contributor Author

GiudGiud commented May 3, 2021

I think it's the right fix for us but if we keep it open it may avoid some confusion later on
Mesh convergence studies are pretty much standard procedure for CFD, so I can see the question come up soon enough

FY 21 NEAMS TH Pronghorn automation moved this from To do to Done May 3, 2021
somu15 pushed a commit to somu15/moose that referenced this issue May 15, 2021
This fixes idaholab#16822 although perhaps not 100% satisfactorily. If a user
knows they only have cells with a maximum of one extrapolated boundary
face or if they know that if the cell has multiple extrapolated boundary
faces that it is orthogonal (e.g. dCf is parallel to Sf), then they can
safely set `two_term_boundary_expansion = true` in their input. But in
the interest of allowing more things to just work out of the box (like
any old tri or tet mesh), I think it makes sense to make the one term
expansion the default. I prefer doing this over something like
projecting dCf onto the surface normal because this would cost a user
accuracy in the case where they have intelligently designed their mesh
such that there is only a maximum of one extrapolated boundary face per
cell (gmsh meshes for example avoid this).
aeslaughter pushed a commit to aeslaughter/moose that referenced this issue Jun 2, 2021
This fixes idaholab#16822 although perhaps not 100% satisfactorily. If a user
knows they only have cells with a maximum of one extrapolated boundary
face or if they know that if the cell has multiple extrapolated boundary
faces that it is orthogonal (e.g. dCf is parallel to Sf), then they can
safely set `two_term_boundary_expansion = true` in their input. But in
the interest of allowing more things to just work out of the box (like
any old tri or tet mesh), I think it makes sense to make the one term
expansion the default. I prefer doing this over something like
projecting dCf onto the surface normal because this would cost a user
accuracy in the case where they have intelligently designed their mesh
such that there is only a maximum of one extrapolated boundary face per
cell (gmsh meshes for example avoid this).
lindsayad added a commit to lindsayad/moose that referenced this issue Sep 28, 2021
This requires implementing try-catch blocks where we first try gradient
computation with the two term boundary expansion and then if we get a
singular system when performing the LU decomposition, then we switch to
a one term boundary expansion just for the currently failing element

Refs idaholab#16822
GiudGiud added a commit to lindsayad/moose that referenced this issue Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: Modules P: normal A defect affecting operation with a low possibility of significantly affects. T: defect An anomaly, which is anything that deviates from expectations.
Development

No branches or pull requests

2 participants