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

Fix cornernode normal #2392

Open
wants to merge 35 commits into
base: develop
Choose a base branch
from
Open

Fix cornernode normal #2392

wants to merge 35 commits into from

Conversation

bigfooted
Copy link
Contributor

@bigfooted bigfooted commented Dec 6, 2024

Proposed Changes

Give a brief overview of your contribution here in a few sentences.
Quick fix to issue #2383
The normal node is the interior node that is most normal to the edge. If the node is on the wall, it is only considered as normal node if no previous normal nodes were detected (just for the case that there are no normal nodes). A normal node that is on the wall can be overwritten by any interior node.

PR Checklist

  • I am submitting my contribution to the develop branch.
  • My contribution generates no new compiler warnings (try with --warnlevel=3 when using meson).
  • My contribution is commented and consistent with SU2 style (https://su2code.github.io/docs_v7/Style-Guide/).
  • I used the pre-commit hook to prevent dirty commits and used pre-commit run --all to format old commits.
  • I have added a test case that demonstrates my contribution, if necessary.
  • I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp), if necessary.

Copy link
Member

@pcarruscag pcarruscag left a comment

Choose a reason for hiding this comment

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

I don't think this is the full fix.
First issue, at vertices shared by two markers we compute two normals for the same point, potentially resulting in different "normal neighbors". Then the result we get depends on whether markers are merged or not, their order, etc.
Then the distance we should use is the wall distance and not just the straight line between points.

So if you want to do something that doesn't break all cases you have the solution I mentioned in the issue

smallest non-zero wall distance of a neighbor of the wall point.

@bigfooted
Copy link
Contributor Author

OK, so the 'normal angle' does not matter anymore in that case, we only have edge length (to an interior node) as requirement?

@stevosg99
Copy link

I am very interested in a solution to this issue, as this particular bug has caused convergence issues for a long time (especially with the SST Models). Here is a comparison of 8.1.0 (with the corner of issues circled in red) and the fix_cornernode_normal version just created.

image

It seems to be causing all of the wall distances to be miscalculated now.

@pcarruscag
Copy link
Member

We only have wall distance as a requirement, not distance between nodes.

@bigfooted
Copy link
Contributor Author

bigfooted commented Dec 13, 2024

OK, I changed it to wall distance.
For the flow over the backward facing step, with a structured mesh as shown, the corner node iPoint does not have any connecting nodes jPoint that are in the interior. In this case, You can loop over the nodes jPoint and check those points for interior points.

But still, the corner node has zero shear stress, so y+ = 0 in that case. But I do not think that this is problematic.

image
image

@pcarruscag
Copy link
Member

Good point, in such cases we can probably use cbrt of the volume at that point to avoid a 0.
My suggestion is to change just the SST wall boundary condition and avoid touching the normal neighbor all together because it impacts too many things in the code.

@bigfooted
Copy link
Contributor Author

I can introduce FindClosest_Neighbor, and use that for the walls in TurbSSTSolver. We can then phase out FindNearest_Neighbor over time.

@pcarruscag
Copy link
Member

Just do what you need to do in the SST wall boundary condition for now

@rois1995
Copy link
Contributor

rois1995 commented Dec 18, 2024

Y plus value of zero just means that the skin friction is zero there, right? which is expected since it is a stagnation point. Thus I don't understand where a division by zero should be occurring. Moreover, why are we computing the distance again to search for the closest one? Shouldn't we use the wall distance already computed?

@bigfooted
Copy link
Contributor Author

@pcarruscag do you think this is the way to go? If so, I can clean it up and fix the regressions, that's still a whole bunch.

@pcarruscag
Copy link
Member

I don't think so.

  • We need the distance projected in the normal direction, not the Euclidean distance.
  • The second search you are using for vertices without an interior neighbor is not valid for MPI, if the vertex is close to an MPI boundary, the second-degree neighbors do not exist.
  • I would rather not add anything to CVertex for now, I'll think about a better way of determining the normal neighbor.

So, in the SST solver, and nowhere else:

  • Loop over the neighbors of the point and find the smallest non-zero wall distance.
  • If that is zero, use 2 * Volume / (sum of vertex areas) as an estimate of the normal distance.

SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Fixed Show fixed Hide fixed
@bigfooted
Copy link
Contributor Author

I updated the regression results. I did not update the restart files, that can be done after the next cornernode PR.

@pcarruscag
Copy link
Member

What is the next corner node PR?

SU2_CFD/include/solvers/CFVMFlowSolverBase.hpp Outdated Show resolved Hide resolved
SU2_CFD/include/solvers/CFVMFlowSolverBase.inl Outdated Show resolved Hide resolved
SU2_CFD/include/solvers/CSolver.hpp Outdated Show resolved Hide resolved
@bigfooted
Copy link
Contributor Author

bigfooted commented Jan 20, 2025

What is the next corner node PR?

We can maybe discuss in the developer meeting what the next step is, what's the best approach to generalize?

@bigfooted bigfooted changed the title [WIP] Fix cornernode normal Fix cornernode normal Jan 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants