-
Notifications
You must be signed in to change notification settings - Fork 849
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
Conversation
There was a problem hiding this 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
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
There was a problem hiding this 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.
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: |
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 |
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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).
They are exactly the same type of solver, we should not have two types of symmetry. |
…ler call at symmetry BC
if ((config->GetKind_Solver() == NEMO_NAVIER_STOKES)) { | ||
if (config->GetKind_Solver() == NEMO_NAVIER_STOKES && config->GetViscous_Wall(iMarker)) { |
There was a problem hiding this comment.
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.
/*--- Correct normal directions of edges ---*/ | ||
for (unsigned long iMarker = 0; iMarker < geometry->GetnMarker(); iMarker++) { | ||
if (config->GetMarker_All_KindBC(iMarker) == SYMMETRY_PLANE){ |
There was a problem hiding this comment.
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.
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. |
Proposed Changes
Implementation of a new boundary condition for symmetry plane instead of using Euler Wall.
The implementation works as follows:
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.
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)
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)