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

[WIP] NEMO - New Symmetry BC #1168

Closed
wants to merge 30 commits into from
Closed

[WIP] NEMO - New Symmetry BC #1168

wants to merge 30 commits into from

Conversation

fmpmorgado
Copy link
Contributor

@fmpmorgado fmpmorgado commented Jan 18, 2021

Proposed Changes

Implementation of a new boundary condition for symmetry plane instead of using Euler Wall.
The implementation works as follows:

  • Calculates new viscous eigenvalues for nodes in symmetry planes;
  • Calculates new residuals to take into account the symmetry of the mesh; Only corrects momentum residual vector in current version.
  • Uses a modified volume to calculate the solution in symmetry planes; Does not require modified volume in current version.

Related Work

#657

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.

Results

For the sake of demonstrating the capability of the new Symmetry Boundary condition, the image shows a comparison of pressure at the surface, using the new implementation and the old symmetry boundary condition, for a flow over a 3D cylinder and considering the lateral and bottom surfaces as symmetry plane:

New Implementation vs Old Implementation for N2 mixture (2nd Order - NEMO_NS solver - AUSM scheme)

Screenshot from 2021-01-20 23-28-31

I have run this test without changing the viscous eigenvalues, and it converges to the same value. However, for unsteady simulations, changing the eigenvalues provides better results.

New Implementation (on NEMO) vs Current SU2 Implementation for AIR5 mixture (2nd Order - NS solver - AUSM scheme)

image

Copy link
Contributor

@CatarinaGarbacz CatarinaGarbacz left a comment

Choose a reason for hiding this comment

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

the implementation seems good to me. some of the comments need to be updated since symmetry is not a boolean. other than that, awesome contribution

SU2_CFD/src/variables/CNEMOEulerVariable.cpp Outdated Show resolved Hide resolved
Comment on lines 538 to 543
if (nodes->GetSymmetry(iPoint))
if(nodes->GetSymmetry(iPoint) == 1 && nodes->GetSymmetry(jPoint)) nodes->AddMax_Lambda_Visc(iPoint, 4*Lambda);
else if (nodes->GetSymmetry(iPoint) == 2 && nodes->GetSymmetry(jPoint) == 1) nodes->AddMax_Lambda_Visc(iPoint, 8*Lambda);
else if (nodes->GetSymmetry(iPoint) == 2 && nodes->GetSymmetry(jPoint) == 2) nodes->AddMax_Lambda_Visc(iPoint, 16*Lambda);
else nodes->AddMax_Lambda_Visc(iPoint, 2*Lambda);
else nodes->AddMax_Lambda_Visc(iPoint, Lambda);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can explain this in the next dev meeting?
This looks so complex that there has to be a simpler way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pcarruscag Sure, when will the next dev meeting be ?

The idea behind it is to mimic the eigenvalues given if using a "full" domain. Probably there is a simpler way through manipulation of the Normal values, but not sure how it can be implemented.

Copy link
Member

Choose a reason for hiding this comment

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

Would doubling at the end be enough?

Wednesdays at 4pm CET https://meet.jit.si/SU2_DevMeeting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the nodes are in a single symmetry plane, yes. A bit more complex when they are in two symmetry planes, but I will look at it.

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.

@fmpmorgado thanks for doing this work! The results are very promising, I think you should add a couple of pictures here to showcase the improvement.

I made a comment about interp, but i think looks pretty good.

SU2_CFD/src/variables/CNEMOEulerVariable.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@jtneedels jtneedels left a comment

Choose a reason for hiding this comment

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

Hi Fabio, thanks for implementing this, it looks really good. Yeah I would second what Wally said, maybe posting the images you showed in the slack as a comment, just to show the difference between the current and former implementations.

@fmpmorgado
Copy link
Contributor Author

fmpmorgado commented Jan 19, 2021

For the sake of demonstrating the capability of the new Symmetry Boundary condition, the image shows a comparison of pressure at the surface, using the new implementation and the old symmetry boundary condition, for a flow over a 3D cylinder and considering the lateral and bottom surfaces as symmetry plane:

5

@fmpmorgado
Copy link
Contributor Author

fmpmorgado commented Jan 21, 2021

After a meeting with @TobiKattmann, I have decided to change NEMO BC_EULER_WALL to match the one in CEulerSolver.cpp, where the velocity vector and gradients are corrected.

Euler boundary condition is now called inside the symmetry plane boundary condition, before performing the residual correction
This way, I believe I am assuring for all the vector quantities to be parallel to the symmetry plane and guaranteeing the condition of symmetry according to Jiri Blazek's - "Computational fluid dynamics: principles and applications" . However, calling Euler function does not appear to substantially change the the solution substantially in the test I have made so far.

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.

@fmpmorgado and I chatted and decided to try out the Version Blazek describes in his book (8.6 in the 3rd edition).

That will also require changing some normal vectors of faces that end on a sym plane (tbd once after mesh reading) and removing components of the gradient for symmetry nodes (tbd each time they get computed). This is not done yet in this PR.

The changes are straight forward once these two mentioned things are done as the sym_plane bc becomes rather short as it just removes the normal residual component (and nothing else). And the Euler wall stays as is so the code needs to be moved over (i.e. again separate implementations for Sym and euler wall). Keeping the viscous stuff has the potential to serve as a slip wall for viscuos flows (if I am not mistaken).

Putting changes to the non-NEMO solvers in this PR is s.th. we could do as the normal-vector and gradient changes should be equal but it would delay the whole PR and a lot of Reg-tests would have changes. AND we have to see whether we have problems in the non-NEMO solvers that would be fixed with this in the first place (correct me if we already know that -> I will test that).

Together with @talbring (thanks!) I briefly looked into the code that Jiri Blazek provides for his book and he implements the boundary he describes (... who would have thought).

Jacobian.AddBlock2Diag(iPoint, residual.jacobian_i);
}

if (viscous) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The viscous part is superflous. (unless you need a slip wall I guess)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right, I have removed the viscous part.


SU2_OMP_FOR_DYN(OMP_MIN_SIZE)
for (iVertex = 0; iVertex < geometry->nVertex[val_marker]; iVertex++) {
if (!preprocessed || geometry->bound_is_straight[val_marker] != true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you delete the viscous part then the tangential-vector computation here is not necessary as well and the section can be simplified.
The bound_is_straight container + evaluation could be deleted in that case as well (unless we want to keep for future ideas.)

su2double *Normal = nullptr, Area, UnitNormal[3], *NormalArea,
**Jacobian_b, **DubDu,
rho, cs, P, rhoE, rhoEve, conc, *u, *dPdU;
BC_Euler_Wall(geometry, solver_container, conv_numerics, visc_numerics, config, val_marker);
Copy link
Contributor

Choose a reason for hiding this comment

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

Euler boundary condition is now called inside the symmetry plane boundary condition, before performing the residual correction
This way, I believe I am assuring for all the vector quantities to be parallel to the symmetry plane and guaranteeing the condition of symmetry according to Jiri Blazek's - "Computational fluid dynamics: principles and applications" . However, calling Euler function does not appear to substantially change the the solution substantially in the test I have made so far.

Imo this is not necessary and that part can be deleted (until Jiri's suggested implementation shows problems).

@pcarruscag
Copy link
Member

Putting changes to the non-NEMO solvers in this PR is s.th. we could do as the normal-vector and gradient changes should be equal but it would delay the whole PR and a lot of Reg-tests would have changes

They are exactly the same type of solver, we should not have two types of symmetry.

if ((config->GetKind_Solver() == NEMO_NAVIER_STOKES)) {
if (config->GetKind_Solver() == NEMO_NAVIER_STOKES && config->GetViscous_Wall(iMarker)) {
Copy link
Member

Choose a reason for hiding this comment

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

I would also remove the check for NEMO_NAVIER_STOKES... This kind of logic causes 90% of the bugs in SU2 every time a new options is added something breaks, the simpler the logic, the easier it is to maintain the code.

Comment on lines +268 to +270
/*--- Correct normal directions of edges ---*/
for (unsigned long iMarker = 0; iMarker < geometry->GetnMarker(); iMarker++) {
if (config->GetMarker_All_KindBC(iMarker) == SYMMETRY_PLANE){
Copy link
Member

Choose a reason for hiding this comment

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

We should probably move this to draft PR while you guys prototype the new symmetry implementation.
Then we can discuss how and where to implement the different changes without copy-pasting code and making sure the fix is compatible with other features of the code.

@fmpmorgado fmpmorgado marked this pull request as draft January 23, 2021 17:39
@pcarruscag pcarruscag changed the title NEMO - New Symmetry BC [WIP] NEMO - New Symmetry BC Feb 8, 2021
@stale
Copy link

stale bot commented Jun 2, 2021

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.

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.

6 participants