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

Cleanup and document the multigrid agglomeration strategy #1372

Merged
merged 7 commits into from Oct 3, 2021

Conversation

pcarruscag
Copy link
Member

Proposed Changes

In trying to understand the algorithm a little better I also tried to improve the documentation and clean up some redundancy.

PR Checklist

  • I am submitting my contribution to the develop branch.
  • 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.

@lgtm-com
Copy link

lgtm-com bot commented Sep 15, 2021

This pull request fixes 7 alerts when merging cb9fce3 into 5ce84cc - view on LGTM.com

fixed alerts:

  • 7 for Comparison of narrow type with wide type in loop condition

@lgtm-com
Copy link

lgtm-com bot commented Sep 29, 2021

This pull request fixes 7 alerts when merging c4f27dc into 6338bff - view on LGTM.com

fixed alerts:

  • 7 for Comparison of narrow type with wide type in loop condition

Copy link
Contributor

@WallyMaier WallyMaier left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for taking the time for cleaning up the code and removing the redundancy!

@pcarruscag pcarruscag merged commit bd586f6 into develop Oct 3, 2021
@pcarruscag pcarruscag deleted the improve_mg branch October 3, 2021 16:33
Copy link
Contributor

@TobiKattmann TobiKattmann left a comment

Choose a reason for hiding this comment

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

Thanks for this cleanup 💐

@@ -264,7 +263,7 @@ class CGeometry {
/*!
* \brief Constructor of the class.
*/
CGeometry(void);
Copy link
Contributor

Choose a reason for hiding this comment

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

Fun Fact, there are 1475 (void) in SU2_CFD and Common ... We/I could make them go extinct (if we are ok with some possible minor merge conflicts in other peoples PR's )

*/
CMultiGridGeometry(CGeometry **geometry, CConfig *config_container, unsigned short iMesh);

private:
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 looks like all methods were public before


/*!
* \brief Determine if a can be agglomerated using geometrical criteria.
* \param[in] iPoint - Seed point.
* \param[in] fine_grid - Geometrical definition of the problem.
* \param[in] config - Definition of the particular problem.
*/
bool GeometricalCheck(unsigned long iPoint, CGeometry *fine_grid, CConfig *config);
bool GeometricalCheck(unsigned long iPoint, const CGeometry *fine_grid, const CConfig *config) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe with that amount of changes clang-format could have been a thing. (Again pointer-Alignment is the rat here)

Comment on lines -43 to -60
unsigned long iPoint, Index_CoarseCV, iElem, iVertex, iteration, nVertexS, nVertexR,
nBufferS_Vector, nBufferR_Vector, iParent, jVertex,Local_nPointCoarse, Local_nPointFine, Global_nPointCoarse, Global_nPointFine,
*Buffer_Receive_Parent = nullptr, *Buffer_Send_Parent = nullptr, *Buffer_Receive_Children = nullptr, *Buffer_Send_Children = nullptr,
*Parent_Remote = nullptr, *Children_Remote = nullptr, *Parent_Local = nullptr, *Children_Local = nullptr;
short marker_seed;
bool agglomerate_seed = true;
unsigned short nChildren, iNode, counter, iMarker, jMarker, priority, MarkerS, MarkerR, *nChildren_MPI;
vector<unsigned long> Suitable_Indirect_Neighbors, Aux_Parent;
vector<unsigned long>::iterator it;

unsigned short nMarker_Max = config->GetnMarker_Max();

unsigned short *copy_marker = new unsigned short [nMarker_Max];

#ifdef HAVE_MPI
int send_to, receive_from;
SU2_MPI::Status status;
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 noice

// unsigned long iPointFree = nPointDomain-1;
// iCoarsePoint = 0;
//
// do {
Copy link
Contributor

Choose a reason for hiding this comment

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

haven't seen a do - while in quite some time.

Good to get rid of those commented code blocks

Comment on lines +1083 to +1087
wall_temperature.fine_grid = fine_grid;
wall_temperature.marker = val_marker;
wall_temperature.target = CustomBoundaryTemperature[val_marker];

SetMultiGridWallQuantity(fine_grid, val_marker, wall_temperature);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice cut-paste-specialize

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.

None yet

3 participants