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

Contiguous storage of CPoint(s) #966

Merged
merged 33 commits into from
May 13, 2020
Merged

Contiguous storage of CPoint(s) #966

merged 33 commits into from
May 13, 2020

Conversation

pcarruscag
Copy link
Member

@pcarruscag pcarruscag commented Apr 30, 2020

Proposed Changes

Along the lines of #753 and #959

If this causes someone lots of conflicts when merging, or you have a lot of new code using the old CPoint (that will not compile anymore) it may be helpful to run this python script (refactor_cpoint.txt) before or after respectively, it was used for 80% of the refactoring done here.

PR Checklist

  • 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).
  • 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.

@pr-triage pr-triage bot added the PR: draft label Apr 30, 2020
@pcarruscag pcarruscag force-pushed the contiguous_cpoint branch from 9f4d95b to 0cbc58e Compare May 1, 2020 13:49
@pcarruscag pcarruscag changed the title Contiguous storage of CPoint(s) [WIP] Contiguous storage of CPoint(s) May 1, 2020
@pcarruscag pcarruscag marked this pull request as ready for review May 1, 2020 16:28
CPoint** node; /*!< \brief Node vector (dual grid information). */
CPoint* nodes; /*!< \brief Node vector (dual grid information). */
Copy link
Member Author

Choose a reason for hiding this comment

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

Usual change no 1

node[iPoint]->SetWall_Distance(val);
nodes->SetWall_Distance(iPoint, val);
Copy link
Member Author

Choose a reason for hiding this comment

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

Usual change no 2.

Comment on lines +50 to +52
CCompressedSparsePatternUL Point; /*!< \brief Points surrounding the central node of the control volume. */
CCompressedSparsePatternL Edge; /*!< \brief Edges that set up a control volume (same sparse structure as Point). */
CCompressedSparsePatternL Elem; /*!< \brief Elements that set up a control volume around a node. */
Copy link
Member Author

Choose a reason for hiding this comment

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

Significant change no 1a: Adjacency information is stored in compressed format (instead of vector of vectors).

Comment on lines -2399 to +2396
node[jPoint] = new CPoint(Local_Coords[iPoint*nDim+0],
Local_Coords[iPoint*nDim+1],
Local_Coords[iPoint*nDim+2],
Local_to_Global_Point[jPoint], config);
nodes->SetCoord(jPoint, &Local_Coords[iPoint*nDim]);
nodes->SetGlobalIndex(jPoint, Local_to_Global_Point[jPoint]);
Copy link
Member Author

Choose a reason for hiding this comment

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

Significant change 2: After allocating the nodes we set data into each instead of instantiating a CPoint object per node.

for (iPoint = 0; iPoint < nPoint; iPoint++)
node[iPoint]->SetnNeighbor(node[iPoint]->GetnPoint());
SU2_OMP_MASTER
nodes->SetPoints(points);
Copy link
Member Author

Choose a reason for hiding this comment

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

Significant change 1b: We set the adjacency for all points in one call, the input is a temporary vector<vector> or similar which the function SetPoints compresses, this is only done in 3 places.

Comment on lines 2897 to 2904
/// TODO: No comms needed for this gradient? The Rmatrix should be allocated somewhere.

/*--- Deallocate memory ---*/
for (iVar = 0; iVar < nDim; iVar++)
delete [] Cvector[iVar];
delete [] Cvector;
const auto& gridVel = geometry->nodes->GetGridVel();
auto& gridVelGrad = geometry->nodes->GetGridVel_Grad();
auto rmatrix = CVectorOfMatrix(nPoint,nDim,nDim);

computeGradientsLeastSquares(nullptr, GRID_VELOCITY, PERIODIC_NONE, *geometry, *config,
true, gridVel, 0, nDim, gridVelGrad, rmatrix);
Copy link
Member Author

Choose a reason for hiding this comment

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

Usual change 4: Grid velocity gradients are no longer duplicated.

Comment on lines -632 to +626
su2double Coord_3D[3];
point_line >> Coord_3D[0]; point_line >> Coord_3D[1]; point_line >> Coord_3D[2];
node[ii] = new CPoint(Coord_3D[0], Coord_3D[1], Coord_3D[2], i, config);
ii++;
break;
}
}
su2double Coord[3] = {0.0};
point_line >> Coord[0];
point_line >> Coord[1];
if (nDim==3) point_line >> Coord[2];
nodes->SetCoord(ii, Coord);
nodes->SetGlobalIndex(ii, i);
Copy link
Member Author

Choose a reason for hiding this comment

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

@vdweide do the DG regressions cover all the types of mesh formats? I think the changes in this PR only affect the DG solver during mesh reading.

@@ -42,211 +42,211 @@ CGridAdaptation::CGridAdaptation(CGeometry *geometry, CConfig *config) {
size = SU2_MPI::GetSize();
rank = SU2_MPI::GetRank();

unsigned long iPoint;
Copy link
Member Author

Choose a reason for hiding this comment

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

40% of the changes are in this file, I had to replace tabs because of Codefactor


Copy link
Member Author

Choose a reason for hiding this comment

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

Another 20% of the changes are here were I accidentally stripped the spaces...

Common/src/geometry/dual_grid/CPoint.cpp Outdated Show resolved Hide resolved
Comment on lines +32 to 33
CPoint::CPoint(unsigned long npoint, unsigned long ndim) : nDim(ndim) {

Copy link
Member Author

@pcarruscag pcarruscag May 3, 2020

Choose a reason for hiding this comment

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

Significant change 3: There is a "minimal constructor" that only allocates the fields needed to read the mesh file (when geometry_aux is built).

@pcarruscag
Copy link
Member Author

Can we get this going folks?

@talbring talbring merged commit a81b0ac into develop May 13, 2020
@talbring talbring deleted the contiguous_cpoint branch May 13, 2020 14:11
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.

3 participants