Reduce flow solver triplication (comp, incomp, nemo) #1177

pcarruscag · 2021-01-23T10:00:40Z

Proposed Changes

Move more routines to the FVM flow solver base class.
At the risk of sounding like a broken record, one function means:
One test, one optimization, one debug, one fix.
Implementing new features by copy-paste-modify is like eating your legs to get to the end of a marathon, it ain't gonna work in the long run xD
Cut, Paste, Abstract, Survive

Related Work

#1044

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

pcarruscag · 2021-01-23T10:04:36Z

SU2_CFD/include/solvers/CFVMFlowSolverBase.hpp

+  template<class SoundSpeedFunc, class LambdaViscFunc>
+  FORCEINLINE void SetTime_Step_impl(const SoundSpeedFunc& soundSpeed,
+                                     const LambdaViscFunc& lambdaVisc,
+                                     CGeometry *geometry,


The idea is to generalize this type of function, the details of the implementation are provided by the derived solver when it instantiates (calls) the generic implementation.
There is no explicit "if incompressible" or "if nemo" or "if compressible" logic.

Thanks for going through this...I made some attempts to update time integration schemes in another branch but didnt push yet.

oops sorry about that, I was looking at this kind of routine and had a "what if" moment... and now it's done

No apologies! Im happy its done.

pcarruscag · 2021-01-23T10:09:01Z

SU2_CFD/src/solvers/CEulerSolver.cpp

+  /*--- Define an object to compute the speed of sound. ---*/
+  struct SoundSpeed {
+    FORCEINLINE su2double operator() (const CEulerVariable& nodes, unsigned long iPoint, unsigned long jPoint) const {
+      return 0.5 * (nodes.GetSoundSpeed(iPoint) + nodes.GetSoundSpeed(jPoint));
    }


The function objects are defined at function scope (SetTime_Step in this example) to make very clear what are the details for each solver.
Using virtual functions or implementing these details in the corresponding variables classes would work, but it would make finding things a lot harder.

I think this looks good. Can you explain the FORCEINLINE?

Hmmm where to start... inline is mostly a mechanism to bypass ODR (one definition rule) if you define a function in an hpp (instead of just declaring), and include that hpp multiple times you are defining the same function multiple times which is not legal, declaring it inline makes it legal (for class methods the rules are less strict).
Because a function is defined inline, the compiler MAY decide (based on a bunch of heuristics I imagine) to copy paste its body into the places where it is used, instead of creating a function call (less indirection and more opportunity to optimize).
Most compilers will have a way for us to ask them to "just do it", i.e. forget about the heuristics and just copy the thing, in SU2 I called that FORCEINLINE.

Summary: FORCEINLINE is triple dog daring the compiler to inline the function.

pcarruscag · 2021-01-23T12:41:15Z

SU2_CFD/include/solvers/CNEMOEulerSolver.hpp

+   * \brief Update the solution using the classical RK4 scheme.
+   * \param[in] geometry - Geometrical definition of the problem.
+   * \param[in] solver_container - Container vector with all the solutions.
+   * \param[in] config - Definition of the particular problem.
+   * \param[in] iRKStep - Runge-Kutta step.
+   */
+  void ClassicalRK4_Iteration(CGeometry *geometry, CSolver **solver_container,
+                              CConfig *config, unsigned short iRKStep) final;


This was missing from NEMO and the incompressible solver, any special reason?

Thats interesting, there shouldn't be a reason to it is missing.

WallyMaier

@pcarruscag this work looks great. Ill be on the lookout to reduce these dup/triplications in future PRs.

WallyMaier · 2021-01-25T05:59:27Z

SU2_CFD/src/solvers/CEulerSolver.cpp

+  /*--- Define an object to compute the speed of sound. ---*/
+  struct SoundSpeed {
+    FORCEINLINE su2double operator() (const CEulerVariable& nodes, unsigned long iPoint, unsigned long jPoint) const {
+      return 0.5 * (nodes.GetSoundSpeed(iPoint) + nodes.GetSoundSpeed(jPoint));
    }


I think this looks good. Can you explain the FORCEINLINE?

WallyMaier · 2021-01-25T06:05:31Z

SU2_CFD/src/solvers/CIncEulerSolver.cpp

+    conv_numerics->SetNormal(Normal);

-      for (iDim = 0; iDim < nDim; iDim++)
-        V_infty[iDim+1] = GetVelocity_Inf(iDim);


These calls V_infty[iDim+1], are one of the main differences between the NEMO solvers and everything. Would it be wasteful to use the the NEMO indices through the code? I haven't looked though enough of the code to know the full impact, but it seems that it could go a long way to reduce duplication.

It would be a little yes, because the velocity offset for nemo is not known at compile-time.

But I have something that might interest you in a subsequent PR.

Oh boy, I cant wait

TobiKattmann

Thanks for these efforts of Cut, Paste, Abstract, Survive 💐 . And yet again: sorry for being too late to the party

(I hope you don't follow all of Mr. Grylls advices, especially in the realm of refreshing beverages 😄 ) (Mr. Grylls's advice? Mr. Grylls' advice? i dont know...)

TobiKattmann · 2021-01-26T09:45:00Z

SU2_CFD/include/solvers/CFVMFlowSolverBase.hpp

+   */
+  void SetResidual_DualTime(CGeometry *geometry, CSolver **solver_container, CConfig *config, unsigned short iRKStep,
+                            unsigned short iMesh, unsigned short RunTime_EqSystem) override;


I wasn't aware that one can withhold the template parameters for the prototype and therefore can overload the function just by using different template parameters https://gcc.godbolt.org/z/9vsEfo . Makes sense though. I learned sth new today :)

That is not exactly what happens there, the method is not a template, the class is.
Each flow solver, incomp, nemo, comp, instantiates a different flavor of CFVMFlowSolverBase.
Each template instantiation creates a different SetResidual_DualTime that overrides the CSolver virtual function.

I don't think you can have method templates of virtual functions, as there would be no way to instantiate them, templates and overloading happen at compile time, and virtual functions at runtime.
That is why it is important to use the virtual specifiers (override, final) to guarantee that a method really overrides something (exact same signature, decide which method to call at runtime based on type) rather than overloading it (different signature, including template params, decide what to call at compile time based on arguments).

What you can have is class templates with virtual functions, each class template instantiation is like a totally different class.

Yep, thanks for the explanation. https://gcc.godbolt.org/z/T6555e this should better reflect the SU2 reality

TobiKattmann · 2021-01-26T10:05:25Z

SU2_CFD/include/solvers/CSolver.hpp

-   */
-  inline virtual void SetBeta_Parameter(CGeometry *geometry,
-                                        CSolver **solver_container,


👍 for putting the inc specific functions where they belong

TobiKattmann · 2021-01-26T10:17:49Z

SU2_CFD/src/solvers/CIncEulerSolver.cpp

    /*--- Check if the node belongs to the domain (i.e., not a halo node) ---*/

-    if (geometry->nodes->GetDomain(iPoint)) {
+    if (!geometry->nodes->GetDomain(iPoint)) continue;


I guess because of this change (and following mass indentation ) the diffs sometimes confusing💫 although from what I see you didn't change a lot more

It was just that, geometry toolbox, and avoiding those old arrays allocated at Solver level ("Residual", "Res_Conv", "Vector", "Primitive" and so on) in preparation for hybrid parallel stuff.

TobiKattmann · 2021-01-26T10:25:37Z

SU2_CFD/src/solvers/CIncEulerSolver.cpp


 void CIncEulerSolver::SetResidual_DualTime(CGeometry *geometry, CSolver **solver_container, CConfig *config,
                                        unsigned short iRKStep, unsigned short iMesh, unsigned short RunTime_EqSystem) {


Ok I guess here it was not straight forward to have that covered in the FVMBase class implementation?

It is a bit more complicated because of the "if energy" and the conversion from primitive to conservative variables

👍 ok I saw the V2U stuff and wondered whether that could be generalized in some way, but if the energy stuff comes on top it might not make too much sense

Yeah it changes whether the loops run to nVar or to nVar-1... It could be done but it felt convoluted.

TobiKattmann · 2021-01-26T10:33:11Z

SU2_CFD/src/solvers/CIncNSSolver.cpp

+void CIncNSSolver::BC_HeatFlux_Wall(CGeometry *geometry, CSolver**, CNumerics*,
+                                    CNumerics*, CConfig *config, unsigned short val_marker) {

-void CIncNSSolver::BC_ConjugateHeat_Interface(CGeometry *geometry, CSolver **solver_container, CNumerics *conv_numerics,
-                                              CConfig *config, unsigned short val_marker) {
+  BC_Wall_Generic(geometry, config, val_marker, HEAT_FLUX);
+}

-  unsigned short iVar, jVar, iDim, Wall_Function;
-  unsigned long iVertex, iPoint, total_index, Point_Normal;
+void CIncNSSolver::BC_Isothermal_Wall(CGeometry *geometry, CSolver**, CNumerics*,
+                                    CNumerics*, CConfig *config, unsigned short val_marker) {

-  su2double *Coord_i, *Coord_j, dist_ij;
-  su2double *GridVel, There, Tconjugate, Twall= 0.0, Temperature_Ref, thermal_conductivity, HF_FactorHere, HF_FactorConjugate;
+  BC_Wall_Generic(geometry, config, val_marker, ISOTHERMAL);
+}



Nice simplification, especially useful if '!energy=true' ;)

TobiKattmann · 2021-01-26T11:00:31Z

TestCases/parallel_regression.py

    channel_2D.cfg_dir   = "sliding_interface/channel_2D"
    channel_2D.cfg_file  = "channel_2D_WA.cfg"
    channel_2D.test_iter = 2
-    channel_2D.test_vals = [2.000000, 0.000000, 0.398053, 0.352779, 0.405462]


I might have missed it in the diff but why did this Testcase change? It is only a minor change but I am curious where it was introduced

The sliding mesh cases are sensitive to very minor things

pcarruscag · 2021-01-26T12:13:18Z

Never late to the party @TobiKattmann nothing is set in stone with git. I will have more incomp changes at some point and was a bit over eager to merge to keep myself from pilling them in just one PR.

pcarruscag added 2 commits January 23, 2021 00:54

avoid duplication of SetTime_Step across the 3 flow solvers

3351a16

cleanup

9f24707

pcarruscag added the changelog:chore label Jan 23, 2021

pr-triage bot added the PR: draft label Jan 23, 2021

pcarruscag commented Jan 23, 2021

View reviewed changes

pcarruscag added 4 commits January 23, 2021 10:35

SetMax_Eigenvalue

d8ef98c

SetCentered_Dissipation_Sensor

95490f3

explicit iterations

51883b8

update channel2D

5cb9f71

pcarruscag commented Jan 23, 2021

View reviewed changes

pcarruscag added 5 commits January 23, 2021 15:00

refactor preprocessing of incompressible solvers

4fddf41

vorticity and strain mag, cleanup IncEuler boundaries

f522924

cleanup CIncNS boundaries not to use the TerribleLegacyVariables

b33d44f

ImplicitEuler_Iteration

f87cd41

IncEuler static arrays of primitives

299f0e0

pcarruscag marked this pull request as ready for review January 23, 2021 19:49

pr-triage bot removed the PR: draft label Jan 23, 2021

pcarruscag changed the title ~~[WIP] Reduce flow solver triplication (comp, incomp, nemo)~~ Reduce flow solver triplication (comp, incomp, nemo) Jan 23, 2021

pr-triage bot added the PR: unreviewed label Jan 23, 2021

pcarruscag added 4 commits January 23, 2021 21:11

fix some virtual specifiers of CNEMOEulerVariable

494b0cb

Avoid member arrays in IncSolver dual-time res

da86881

fix leak in LoadRestart

71bfe14

dual time residual seems to be the same for NEMO

6d12484

WallyMaier approved these changes Jan 25, 2021

View reviewed changes

pr-triage bot added PR: reviewed-approved and removed PR: unreviewed labels Jan 25, 2021

Merge branch 'develop' into reduce_more_duplication

5914c1f

pr-triage bot added PR: unreviewed and removed PR: reviewed-approved labels Jan 25, 2021

pcarruscag merged commit 0baf91d into develop Jan 25, 2021

pcarruscag deleted the reduce_more_duplication branch January 25, 2021 10:02

pr-triage bot added PR: merged PR: unreviewed and removed PR: unreviewed PR: merged labels Jan 25, 2021

TobiKattmann reviewed Jan 26, 2021

View reviewed changes

TobiKattmann added a commit that referenced this pull request Jan 27, 2021

Fix Reg test and fix insufficient of #1177

1d058cc


		void CIncEulerSolver::SetResidual_DualTime(CGeometry geometry, CSolver solver_container, CConfig config,
		unsigned short iRKStep, unsigned short iMesh, unsigned short RunTime_EqSystem) {

Reduce flow solver triplication (comp, incomp, nemo) #1177

Reduce flow solver triplication (comp, incomp, nemo) #1177

Uh oh!

Conversation

pcarruscag commented Jan 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Related Work

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WallyMaier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TobiKattmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcarruscag commented Jan 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pcarruscag commented Jan 23, 2021 •

edited

Loading