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

Illegal access to normal neighbor of halo vertex #1430

Closed
maxaehle opened this issue Nov 9, 2021 · 9 comments · Fixed by #1362
Closed

Illegal access to normal neighbor of halo vertex #1430

maxaehle opened this issue Nov 9, 2021 · 9 comments · Fixed by #1362

Comments

@maxaehle
Copy link
Contributor

maxaehle commented Nov 9, 2021

Describe the bug
In CFVMFlowSolverBase<>::Friction_Forces, there is the following piece of code (permalink):

    /*--- Loop over the vertices to compute the forces ---*/
    
    for (iVertex = 0; iVertex < geometry->nVertex[iMarker]; iVertex++) {
      iPoint = geometry->vertex[iMarker][iVertex]->GetNode();
      iPointNormal = geometry->vertex[iMarker][iVertex]->GetNormal_Neighbor();
      ...
    }

However, a normal neighbor exists only for vertices on the domain (see CMultiGridGeometry::FindNormal_Neighbor and CPhysicalGeometry::FindNormal_Neighbor), not for halo points.
Edit: The link for CPhysicalGeometry was wrong. CPhysicalGeometry actually computes the normal neighbors for halo points, CMultigridGeometry doesn't.

iPointNormal is needed to retrieve Coord_Normal, which is needed to compute WallDist/WallDistMod, which are needed to compute YPlus. To fix the above problem, I would therefore suggest to compute YPlus only for domain points.

Looking for locations where YPlus could be accessed (to see whether YPlus is needed on non-domain points), I found e.g. CTurbSSTSolver::SetTurbVars_WF. However there again the normal neighbor is accessed without checking that we're on a domain point, so likely this should also be changed.

@pcarruscag
Copy link
Member

Have you checked if you ever get halo point in normal markers?

@maxaehle
Copy link
Contributor Author

maxaehle commented Nov 9, 2021

Yes, in the testcase described here: #1362 (comment) I saw in the debugger that halo vertices occur in the loop.

@pcarruscag
Copy link
Member

Right, in the viscous forces case, it should be ok to move the "if Domain" up and avoiding computations on halo points.

@pcarruscag
Copy link
Member

I guess this is a problem for FULL MG only, because CPhysicalGeometry finds normal neighbors also for halo points.

@pcarruscag
Copy link
Member

@bigfooted , I see we are skiping halo nodes when we compute TauWall via wall functions, probably also because of this PointNormal problem?
This may be a problem, because some edges might be missing one tau wall when computing the viscous fluxes.
If that is the case the easier solution is to communicate TauWall.

@maxaehle
Copy link
Contributor Author

By the way, is there a reason why CMultiGridGeometry::FindNormal_Neighbor does not compute the normal neighbor for halo nodes (if anyone happens to know)?
screenshot

@maxaehle
Copy link
Contributor Author

in the viscous forces case, it should be ok to move the "if Domain" up and avoiding computations on halo points.

Corrected in commit f6f986e in #1362 similar to that, moving the code depending on the nearest neighbor access (i.e. YPlus calculation) down into an "if Domain" block. Note however that after this change YPlus is not computed on halo cells any more, which might be problematic e.g. for CTurbSSTSolver::SetTurbVars_WF.

@pcarruscag
Copy link
Member

Maybe it has something to do with the weird shapes of agglomerated volumes, which could make it difficult to find a consistent normal neighbor across ranks.

When Wall Functions are used, y+ is computed elsewhere and not in Viscous forces, and that elsewhere function is already skiping halos, hence my earlier comment on potentially having to communicate tau wall, etc.

@stale stale bot added the stale label Mar 2, 2022
@su2code su2code deleted a comment from stale bot Mar 2, 2022
@stale stale bot removed the stale label Mar 2, 2022
@stale
Copy link

stale bot commented May 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still a relevant issue please comment on it to restart the discussion. Thank you for your contributions.

@stale stale bot added the stale label May 2, 2022
@pcarruscag pcarruscag linked a pull request May 15, 2022 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants