Skip to content

Conversation

@pcarruscag
Copy link
Member

@pcarruscag pcarruscag commented Apr 28, 2020

Proposed Changes

Make CEdge a class of arrays, instead of using it as an array of classes.
Ended up having to add some const-correctness to CNumerics too.
Forgot to turn off "auto blank space stripping" so a few files will show more changes than this actually introduces.

Related Work

Needed for #789 (to eventually support the SIMD part).
Similar changes to #753.
Fixes a bug in the SST fluid-fluid interface.

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.

Copy link
Member Author

@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.

The gist of the PR:

CPrimalGrid*** bound; /*!< \brief Boundary vector (primal grid information). */
CPoint** node; /*!< \brief Node vector (dual grid information). */
CEdge** edge; /*!< \brief Edge vector (dual grid information). */
CEdge* edges; /*!< \brief Edge 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.

Significant change 1.

Comment on lines -406 to +407
Index_t iPoint = geometry.edge[iEdge]->GetNode(0);
Index_t jPoint = geometry.edge[iEdge]->GetNode(1);
Index_t iPoint = geometry.edges->GetNode(iEdge,0);
Index_t jPoint = geometry.edges->GetNode(iEdge,1);
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.

Comment on lines +501 to +503
/*--- Menter's first blending function ---*/

if(sst) visc_numerics->SetF1blending(nodes->GetF1blending(iPoint), nodes->GetF1blending(iPoint));
Copy link
Member Author

@pcarruscag pcarruscag Apr 29, 2020

Choose a reason for hiding this comment

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

The F1 blending factor was missing in BC_Fluid_Interface of the SST solver (classic copy paste bug...), which is what was causing a bunch of valgrind warnings.
I moved the entire method to CTurbSolver, I rather have one if(sst) than 1 copy pasted method.

By the way, the way this method includes the viscous residual is very fishy... But that is not something I will fix.

@pcarruscag pcarruscag mentioned this pull request Apr 30, 2020
5 tasks
Copy link
Member

@talbring talbring left a comment

Choose a reason for hiding this comment

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

Thanks @pcarruscag! All of the changes make sense. Just two very small questions below.

for (unsigned short iDim = 0; iDim < nDim; iDim++)
Normal[iDim]=val_face_normal[iDim];
template<class T>
void SetNormal(unsigned long iEdge, const T& normal) {
Copy link
Member

Choose a reason for hiding this comment

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

Why are they templated? To pass different array types? 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

That was the idea yes, in case we need to pass std::vector or std::array or even just an object with operator []

@pcarruscag
Copy link
Member Author

I'll merge this one to make the diff on my other PR's cleaner, but feel free to comment if you see something that does not look right.
I think these are fairly safe changes bug-wise, CEdge does not have a lot of methods and so I did this almost manually.

@pcarruscag pcarruscag merged commit ec34588 into develop May 1, 2020
@pcarruscag pcarruscag deleted the contiguous_cedge branch May 1, 2020 18:05
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