Fix the neighbor-finding in `CInterpolator::ReconstructBoundary` #1346

maxaehle · 2021-08-04T11:55:51Z

Proposed Changes

CInterpolator::ReconstructBoundary builds arrays Buffer_Receive_StartLinkedNodes, _LinkedNodes,... that store the connectivity of the marker, i.e. the list of neighbor vertices for each vertex.

In the previous version of the code, two vertices were considered "neighbors" if they were connected through an edge of a volume element ("volume neighbors"). We propose to consider vertices as "neighbors" only if they are connected through an edge of a surface element ("surface neighbors").

These two notions are equivalent for smooth surfaces and clearly surface neighbors are always volume neighbors as well. However, at surfaces with sharp edges, two vertices can be volume neighbors without being surface neighbors. This happens when the edge of the volume element connecting the vertices lies inside the domain.

CInterpolator::ReconstructBoundary is called from CSlidingMesh::SetTransferCoeff, which then hands the connectivity structure to CSlidingMesh::Build_3D_surface_element. The latter function forms a circular ordering of the neighbors of each vertex s: e.g. if there are four neighbors a,b,c,d of s with connectivity a-b,c-d,b-c,a-d then it performs an iteration over e.g. a-b-c-d. If we understand "neighbors" as "surface neighbors", the neighbors of any vertex s can be brought into a "closed circle" or "open chain" ordering where each of them is neighbor only to its direct predecessor and successor. This is not true for the volume neighbor relation however, and in this case the iteration might enter an infinite loop e-a-b-c-d-a-b-... (until the access to element[ iElementNode ] segfaults). That's approximately how we (@maxaehle,@mcmorelli,@BeckettZhou,@EduardoMolina) noticed the bug.

Related Work

#1344

PR Checklist

Put an X by all that apply. You can fill this out after submitting the PR. If you have any questions, don't hesitate to ask! We want to help. These are a guide for you to know what the reviewers will be looking for in your contribution.

I am submitting my contribution to the develop branch.
My contribution generates no new compiler warnings (try with the '-Wall -Wextra -Wno-unused-parameter -Wno-empty-body' compiler flags, or simply --warnlevel=2 when using meson).
My contribution is commented and consistent with SU2 style.
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.

pcarruscag

Thank you for looking into this, these interpolation functions can get quite complicated.
I think there is some room to simplify the new algorithm a bit, and to improve the old code if you have some time.
Just before merging this PR please run clang-format on CInterpolator.cpp.

Common/src/interface_interpolation/CInterpolator.cpp

Common/src/interface_interpolation/CNearestNeighbor.cpp

Common/src/interface_interpolation/CInterpolator.cpp

Common/include/interface_interpolation/CSlidingMesh.hpp

Common/src/interface_interpolation/CInterpolator.cpp

Co-authored-by: Pedro Gomes <[email protected]>

Common/src/interface_interpolation/CInterpolator.cpp

…ary_pr2' into fix_slidingmesh_reconstructboundary_pr2

instead of raw array long*

pcarruscag

Thanks, run clang-format on CInterpolator.cpp before merging please, it's a bit messy with unaligned comments, etc.

TobiKattmann · 2021-08-14T11:10:24Z

That's really strange: The hybrid AD testcase discadj_incomp_turb_NACA0012_sst was ok after commit c68cf13, then I changed the stored residuals of unrelated testcases in 9d8f194 and the testcase fails... I'll run clang-format now, let's see what happens ;)

I had a similar thing over at the flamelet branch and a re-run of the regression tests solved that :)

TobiKattmann

Thanks @maxaehle 💐 ... just a little nagging in a few places.

Common/include/interface_interpolation/CInterpolator.hpp

Common/src/interface_interpolation/CSlidingMesh.cpp

TobiKattmann · 2021-08-14T11:19:05Z

Common/src/interface_interpolation/CSlidingMesh.cpp

+              donor_iMidEdge_point[iDim] = ( DonorPoint_Coord[ donor_forward_point ][ iDim] +
+                                             DonorPoint_Coord[ donor_iPoint ][ iDim] ) / 2;
+              donor_jMidEdge_point[iDim] = ( DonorPoint_Coord[ donor_backward_point ][ iDim] +
+                                             DonorPoint_Coord[ donor_iPoint ][ iDim] ) / 2;


Suggested change

donor_iMidEdge_point[iDim] = ( DonorPoint_Coord[ donor_forward_point ][ iDim] +

DonorPoint_Coord[ donor_iPoint ][ iDim] ) / 2;

donor_jMidEdge_point[iDim] = ( DonorPoint_Coord[ donor_backward_point ][ iDim] +

DonorPoint_Coord[ donor_iPoint ][ iDim] ) / 2;

donor_iMidEdge_point[iDim] = ( DonorPoint_Coord[donor_forward_point][iDim] +

DonorPoint_Coord[donor_iPoint][iDim] ) / 2;

donor_jMidEdge_point[iDim] = ( DonorPoint_Coord[donor_backward_point][iDim] +

DonorPoint_Coord[donor_iPoint][iDim] ) / 2;

Any reason to not use zero-spacing (like suggested) for []-access? I do understand how thats helpful for nested accesses [[][]]. But there is no consistency with the additions here [ iDim] [ iDim ] element[0][iDim] = coord[centralNode ][ iDim]. plz consolidate

The right thing there would be operator ( , ) because Max replaced pointers with containers.
If we just want to clean the spaces then run clang-format and call it a day.

I forgot to replace by operator(i,j) here, fixed in 475c697

Common/src/interface_interpolation/CInterpolator.cpp

TobiKattmann · 2021-08-14T11:40:33Z

Common/src/interface_interpolation/CInterpolator.cpp

+    iPoint = geom->vertex[val_marker][iVertex]->GetNode();
+    if (geom->nodes->GetDomain(iPoint)) {
+      unsigned long iLocalVertex = iVertex_to_iLocalVertex[iVertex];


Suggested change

iPoint = geom->vertex[val_marker][iVertex]->GetNode();

if (geom->nodes->GetDomain(iPoint)) {

unsigned long iLocalVertex = iVertex_to_iLocalVertex[iVertex];

const auto iPoint = geom->vertex[val_marker][iVertex]->GetNode();

if (geom->nodes->GetDomain(iPoint)) {

const auto iLocalVertex = iVertex_to_iLocalVertex[iVertex];

not sure if the const for the iPoint is possible but that would be prefered.

This approach would be appreciated in the rest the blocks as well not just here

I replaced unsigned long by const auto at some locations

TobiKattmann · 2021-08-14T11:43:16Z

Common/src/interface_interpolation/CInterpolator.cpp


-  unsigned long iVertex, jVertex, kVertex;
+  unsigned long iVertex, kVertex;


😞 not very gut

I always prefer to keep those loop variables local to their scope, independent of the number of occurrences in the function. This locality makes it easier imo to modify things because block are somewhat more self-contained and one can be sure that no-one does some shenanigans with break where the value has to leave its loop scope. I also like to do it with stuff like iPoint (see below) for the same reasons. I did not get hit by @pcarruscag with the performance-club yet so I expect that the compiler does efficient things with memory (de)allocation

Here i mean of course not the change you did, but keeping i,kVertex

Those are either on the stack or probably don't even leave the registers.

I completely agree with @TobiKattmann that these big declaration blocks in the beginning of a function are not very meaningful to the reader. As according to @pcarruscag there is no performance concern, I improved this in CInterpolator::ReconstructBoundary and CSlidingMesh::Build_3D_surface_element.

Common/src/interface_interpolation/CInterpolator.cpp

TobiKattmann · 2021-08-14T11:55:40Z

Common/src/interface_interpolation/CInterpolator.cpp

+  /*--- Get the number of domain vertices on the marker, and a mapping
+   * (iVertex) -> (iLocalVertex, non-domain points being ignored). ---*/
+  su2vector<unsigned long> iVertex_to_iLocalVertex(nVertex);
+  nLocalVertex = 0;


declare it here if it is needed here for the first time

Common/src/interface_interpolation/CInterpolator.cpp

pcarruscag · 2021-08-14T12:20:18Z

Hybrid AD tests are a bit flaky ATM 🤷 you can merge things without those tests passing, but if more than 1 fails let me know.

Update Common/src/interface_interpolation/CSlidingMesh.cpp Co-authored-by: TobiKattmann <[email protected]>

Update Common/src/interface_interpolation/CInterpolator.cpp Co-authored-by: TobiKattmann <[email protected]>

maxaehle · 2021-08-16T06:43:32Z

Common/src/interface_interpolation/CInterpolator.cpp

-          nNodes++;
+  /*--- Define the neighbors map. ---*/
+  for (unsigned long iElem = 0; iElem < nElems; iElem++) {
+    CPrimalGrid* elem = geom->bound[val_marker][iElem];


This cannot be made const CPrimalGrid* elem at the moment because CPrimalGrid::GetNode etc are not marked as const.

I guess/assume you could make the various GetNode functions const :) ... and then const auto elem ... but this goes a little beyond PR scope so up to you

TobiKattmann · 2021-08-16T07:23:05Z

Common/src/interface_interpolation/CSlidingMesh.cpp

-            target_jMidEdge_point[iDim] = ( TargetPoint_Coord[ target_segment[1] ][ iDim ] +
-                                            target_geometry->nodes->GetCoord( target_iPoint , iDim) ) / 2;
+            target_iMidEdge_point[iDim] = ( TargetPoint_Coord(target_segment[0], iDim ) +
+                                            target_geometry->nodes->GetCoord( target_iPoint , iDim) ) / 2.;


I am curious: Did you get a compiler notification or so on this 2. or was it just personal preference?

Personal preference. Once I spent a lot of time finding out that 1/2==0.

maxaehle · 2021-08-16T07:49:35Z

That's really strange: The hybrid AD testcase discadj_incomp_turb_NACA0012_sst was ok after commit c68cf13, then I changed the stored residuals of unrelated testcases in 9d8f194 and the testcase fails... I'll run clang-format now, let's see what happens ;)

I had a similar thing over at the flamelet branch and a re-run of the regression tests solved that :)

Seems to be like that here as well:
After e95cdfa, only transonic_stator in hybrid_regression_AD.py failed.
After 475c697, only discadj_incomp_turb_NACA0012_sa in hybrid_regression_AD.py failed.
After b704f9e no test in hybrid_regression_AD.py failed.

Common/include/interface_interpolation/CInterpolator.hpp

Update Common/include/interface_interpolation/CInterpolator.hpp Co-authored-by: Pedro Gomes <[email protected]>

maxaehle · 2021-08-16T13:29:46Z

After 24d81f5, only ea_naca64206 in parallel_regression_AD.py failed

throw out changes independent from the surface-neighbor PR

87f8cc6

pr-triage bot added the PR: unreviewed label Aug 4, 2021

removed unused variable received_nLocalLinkedNodes_sum

491c4d3

pcarruscag reviewed Aug 4, 2021

View reviewed changes

maxaehle added 3 commits August 5, 2021 10:19

iVertex_to_iLocalVertex as su2vector

332ca23

Buffer_Send_Coord, Buffer_Receive_Coord as su2activematrix

30ac1aa

C-style cast -> static_cast

19d7a95

maxaehle commented Aug 5, 2021

View reviewed changes

Common/src/interface_interpolation/CNearestNeighbor.cpp Outdated Show resolved Hide resolved

pcarruscag reviewed Aug 5, 2021

View reviewed changes

Common/src/interface_interpolation/CInterpolator.cpp Outdated Show resolved Hide resolved

map of forward_list's -> unordered_map of unordered_set's

480d9a3

pcarruscag reviewed Aug 5, 2021

View reviewed changes

Common/include/interface_interpolation/CSlidingMesh.hpp Outdated Show resolved Hide resolved

pcarruscag reviewed Aug 5, 2021

View reviewed changes

Common/src/interface_interpolation/CInterpolator.cpp Outdated Show resolved Hide resolved

const before type

12b4b8a

Co-authored-by: Pedro Gomes <[email protected]>

pcarruscag reviewed Aug 5, 2021

View reviewed changes

Common/src/interface_interpolation/CInterpolator.cpp Outdated Show resolved Hide resolved

maxaehle added 8 commits August 5, 2021 12:24

access su2matrix via (i,j) not [i][j] interface

7cc51eb

Merge remote-tracking branch 'origin/fix_slidingmesh_reconstructbound…

675e00f

…ary_pr2' into fix_slidingmesh_reconstructboundary_pr2

continue 7cc51eb

b60c994

Buffer_Receive_Proc as su2vector

304c3c3

Buffer_Receive_LinkedNodes, Buffer_Send_LinkedNodes as su2vector

ee8a905

Buffer_Receive_GlobalPoint as su2vector<unsigned long>

612be75

instead of raw array long*

Buffer_Receive_nLinkedNodes as su2vector

969f762

Buffer_Receive_StartLinkedNodes as su2vector

a76a838

pcarruscag added the changelog:fix label Aug 5, 2021

pcarruscag approved these changes Aug 5, 2021

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: unreviewed labels Aug 5, 2021

Fix size for Allgather

7418be2

pr-triage bot added PR: unreviewed and removed PR: reviewed-approved labels Aug 6, 2021

MPI_LONG -> MPI_UNSIGNED_LONG where type changed in 612be75

c776545

TobiKattmann approved these changes Aug 14, 2021

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: unreviewed labels Aug 14, 2021

unsigned long -> auto

892ef65

Update Common/src/interface_interpolation/CSlidingMesh.cpp Co-authored-by: TobiKattmann <[email protected]>

pr-triage bot added PR: unreviewed and removed PR: reviewed-approved labels Aug 16, 2021

maxaehle and others added 7 commits August 16, 2021 07:59

unsigned long -> auto

e95cdfa

Update Common/src/interface_interpolation/CSlidingMesh.cpp Co-authored-by: TobiKattmann <[email protected]>

declare variable nDim when defining it

83c7a90

Update Common/src/interface_interpolation/CInterpolator.cpp Co-authored-by: TobiKattmann <[email protected]>

Declare variables closer to their initialization

12d132e

// -> /*--- ---*/

da41cdb

[i][j] -> operator(i,j) for su2activematrix

475c697

continue 475c697

453388d

more const auto

9965b85

maxaehle commented Aug 16, 2021

View reviewed changes

move variable decl in CSlidingMesh::Build_3D_surface_element

2411b96

TobiKattmann reviewed Aug 16, 2021

View reviewed changes

continue polishing CSlidingMesh::Build_3D_surface_element

b704f9e

pcarruscag reviewed Aug 16, 2021

View reviewed changes

Common/include/interface_interpolation/CInterpolator.hpp Outdated Show resolved Hide resolved

remove unnecessary #include <forward_list>

24d81f5

Update Common/include/interface_interpolation/CInterpolator.hpp Co-authored-by: Pedro Gomes <[email protected]>

pcarruscag merged commit 1a84301 into develop Aug 16, 2021

pcarruscag deleted the fix_slidingmesh_reconstructboundary_pr2 branch August 16, 2021 13:45

pr-triage bot added PR: merged and removed PR: unreviewed labels Aug 16, 2021

maxaehle mentioned this pull request Aug 16, 2021

Refactor CPrimalGrid classes #1352

Merged

5 tasks

pr-triage bot added PR: unreviewed and removed PR: merged labels Aug 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the neighbor-finding in `CInterpolator::ReconstructBoundary` #1346

Fix the neighbor-finding in `CInterpolator::ReconstructBoundary` #1346

maxaehle commented Aug 4, 2021 •

edited

Loading

pcarruscag left a comment

pcarruscag left a comment

TobiKattmann commented Aug 14, 2021

TobiKattmann left a comment

TobiKattmann Aug 14, 2021

TobiKattmann Aug 14, 2021

pcarruscag Aug 14, 2021

maxaehle Aug 16, 2021

TobiKattmann Aug 14, 2021

TobiKattmann Aug 14, 2021

maxaehle Aug 16, 2021

TobiKattmann Aug 14, 2021

TobiKattmann Aug 14, 2021

pcarruscag Aug 14, 2021

maxaehle Aug 16, 2021

TobiKattmann Aug 14, 2021

pcarruscag commented Aug 14, 2021

maxaehle Aug 16, 2021

TobiKattmann Aug 16, 2021

maxaehle Aug 17, 2021

TobiKattmann Aug 16, 2021

maxaehle Aug 16, 2021

maxaehle commented Aug 16, 2021

maxaehle commented Aug 16, 2021


		unsigned long iVertex, jVertex, kVertex;
		unsigned long iVertex, kVertex;

Fix the neighbor-finding in CInterpolator::ReconstructBoundary #1346

Fix the neighbor-finding in CInterpolator::ReconstructBoundary #1346

Conversation

maxaehle commented Aug 4, 2021 • edited Loading

Proposed Changes

Related Work

PR Checklist

pcarruscag left a comment

Choose a reason for hiding this comment

pcarruscag left a comment

Choose a reason for hiding this comment

TobiKattmann commented Aug 14, 2021

TobiKattmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcarruscag commented Aug 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxaehle commented Aug 16, 2021

maxaehle commented Aug 16, 2021

Fix the neighbor-finding in `CInterpolator::ReconstructBoundary` #1346

Fix the neighbor-finding in `CInterpolator::ReconstructBoundary` #1346

maxaehle commented Aug 4, 2021 •

edited

Loading