Restructuring of solver files #849

jayantmukho · 2020-01-11T08:45:05Z

Proposed Changes

This is a restructuring of the solver classes in the fashion of what has been done for variable classes, output classes etc. Changes include:

Moving all solver .hpp and .cpp files into individual class specific files
Moving all inline functions into the header files
Some bits of code cleanup

Things still left to be done:

Updating the IDE files
Any other things that might come up in the review process

This has mostly just been copying and pasting things around. If there any thing else to address in the solver classes (menial or significant) that would be appropriate to include in this restructuring, let me know in the comments!

Related Work

@pcarruscag is already working on the CFEASolver in #843. I have split the files here as well since it was already automated. But I can just merge with his branch in the likely case that his PR is ready for merging before this one.

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

Nice work @jayantmukho, I hope that script helped with moving the inlines.
General question, did you update the #include "solver_structure.hpp" automatically or manually (fixing the compilation errors one by one)? I ask because this kind of file restructuring gives us a good opportunity to make sure we are including files only where needed, which helps with build times.

Apart from the two comments below, I would add that it would be nice to put virtual (override or final) and const specifiers where appropriate, at some point, not necessarily now.

SU2_DOT/src/meson.build

SU2_CFD/include/SU2_CFD.hpp

jayantmukho · 2020-01-13T16:28:42Z

Nice work @jayantmukho, I hope that script helped with moving the inlines.

Yes! It did. I was able to build off the script you gave me to enable moving multi-line inlines. Now it only fails when the function signatures don't match up (because of different variable names). I've attached it here:
split_inlines.txt
merge_inlines.txt

General question, did you update the #include "solver_structure.hpp" automatically or manually (fixing the compilation errors one by one)? I ask because this kind of file restructuring gives us a good opportunity to make sure we are including files only where needed, which helps with build times.

I replaced the solver_structure.hpp files with just CSolver.hpp first. Then quashed errors by specific solver includes when needed. But its a good point. I will go through it again to remove any unnecessary includes that might have crept in.

Apart from the two comments below, I would add that it would be nice to put virtual (override or final) and const specifiers where appropriate, at some point, not necessarily now.

Ah yes, this is a good time to do this. Will do.

talbring · 2020-01-14T13:10:45Z

@jayantmukho Thanks for that restructuring. When you are finished use the two commands in the new folders from the script externals/utils/replace-tabs-and-trim-trailing-whitespaces.sh to replace tabs and to remove whitespaces.

This is probably the part where most of the people will encounter conflicts and problems. I can only encourage everybody to merge with develop ASAP. It won't get any easier.

SU2_CFD/include/solvers/CAdjTurbSolver.hpp

…l_and_SIMD_tmp

…ure_hybrid_parallel_and_SIMD_tmp

jayantmukho

Some things came up while going through with a fine tooth comb. A couple of unused functions, and an unused boundary condition

jayantmukho · 2020-01-23T22:42:12Z

SU2_CFD/include/solvers/CSolver.hpp

+   * \param[in] val_coord - Location (x, y, z) of the max residual point.
+   */
+  // inline void AddRes_Max(unsigned short val_var, su2double val_residual, unsigned long val_point, su2double* val_coord) {
+  //   if (val_residual > Residual_Max[val_var]) {


There are a couple of functions like this that I found no references to elsewhere in the code. So I have them commented out for now. Will be removing them soon.

You can find the functions by searching for TODO. Some in CSolver, CEulerSolver, CIncEulerSolver

jayantmukho · 2020-01-23T22:45:17Z

SU2_CFD/include/solvers/CSolver.hpp

+   * \param[in] val_marker - Surface marker where the boundary condition is applied.  
+   */ 
+  // inline virtual void BC_Neumann(CGeometry *geometry,
+  //                                CSolver **solver_container,


This boundary condition (BC_Neumann) doesn't seem to be implemented anywhere. It is also not mentioned in the config template.

Unless there is a plan for this, I was going to take out all references to the Neumann boundary conditions. Including the option from config_structure.cpp.

Maybe it is from the time when there was a Poisson solver?

It is. The BC_Dirichlet was also from the Poisson solver, not sure if it is used elsewhere.

@koodlyakshay you are right, the BC_Dirichlet is just an empty function in CEulerSolver.

I'll remove both. Thanks for the input!

jayantmukho · 2020-01-23T23:04:20Z

SU2_CFD/src/integration_structure.cpp

-        break;
+      // case NEUMANN: TODO
+      //   solver_container[MainSolver]->BC_Neumann(geometry, solver_container, numerics[CONV_BOUND_TERM], config, iMarker);
+      //   break;


This is the only call to BC_Neumann

SU2_CFD/include/solvers/CSolver.hpp

…ure_hybrid_parallel_and_SIMD_tmp

…arallel_and_SIMD_tmp

jayantmukho · 2020-01-24T20:12:03Z

Thanks for taking care of the merge with develop @pcarruscag, I am going to make some simple formatting changes to the CFEASolver and CMeshSolver so that they conform to the rest of the solver files

jayantmukho

I still have to change the visual studio and xcode project files. I haven't done this before so if anyone has any tips, links, how-to's, that'd be greatly appreciated.

jayantmukho · 2020-01-24T22:07:14Z

SU2_CFD/include/gradients/computeGradientsLeastSquares.hpp

        /*--- Distance vector from iPoint to jPoint ---*/

-        su2double dist_ij[MAXNDIM];
+        su2double dist_ij[MAXNDIM] = {0.0};


Just getting rid of some Wmaybe-uninitialized flags

jayantmukho · 2020-01-24T22:13:39Z

SU2_CFD/include/solvers/CFEASolver.hpp

+                     unsigned short iMesh,
+                     unsigned long Iteration,
+                     unsigned short RunTime_EqSystem,
+                     bool Output) override;


This is the formatting I had mentioned. I'd changed all the other solver files so I figured CFEASolver and CMeshSolver can join in the makeover

jayantmukho · 2020-01-24T22:15:45Z

SU2_CFD/include/solvers/CFEASolver.hpp

+  inline void BC_Normal_Displacement(CGeometry *geometry,
+                                     CNumerics *numerics,
+                                     CConfig *config,
+                                     unsigned short val_marker) final { }


@pcarruscag I'm assuming you have some plan for this and BC_Sine_Load functions which is why these are the only definitions of them

jayantmukho · 2020-01-24T22:15:58Z

SU2_CFD/include/solvers/CFEASolver.hpp

+  inline void BC_Sine_Load(CGeometry *geometry,
+                           CNumerics *numerics,
+                           CConfig *config,
+                           unsigned short val_marker) final { }


jayantmukho · 2020-01-24T22:16:34Z

SU2_CFD/include/solvers/CFEASolver.hpp

+                   CSolver ***solver,
+                   CConfig *config,
+                   int val_iter,
+                   bool val_update_geo) override;


added an override over here

SU2_CFD/include/solvers/CSolver.hpp

jayantmukho · 2020-01-24T22:26:40Z

SU2_CFD/src/interfaces/cht/CConjugateHeatInterface.cpp

-               || (donor_config->GetKind_Solver() == DISC_ADJ_INC_RANS)
-               && (donor_config->GetEnergy_Equation()));
+               || (donor_config->GetKind_Solver() == DISC_ADJ_INC_RANS))
+               && donor_config->GetEnergy_Equation());


This is purely to get rid the last pesky -Wparentheses warning. Logic is still the same

Don't suppress that warning until someone says something about it, I left a post merge comment in #839, the logic looks fishy.

pcarruscag · 2020-01-26T11:07:16Z

SU2_CFD/include/drivers/CDriver.hpp

+#include "../solvers/CSolver.hpp"
+#include "../solvers/CEulerSolver.hpp"
+#include "../solvers/CIncEulerSolver.hpp"
+#include "../solvers/CNSSolver.hpp"
+#include "../solvers/CIncNSSolver.hpp"
+#include "../solvers/CTurbSASolver.hpp"
+#include "../solvers/CTurbSSTSolver.hpp"
+#include "../solvers/CTransLMSolver.hpp"
+#include "../solvers/CAdjEulerSolver.hpp"
+#include "../solvers/CAdjNSSolver.hpp"
+#include "../solvers/CAdjTurbSolver.hpp"
+#include "../solvers/CHeatSolverFVM.hpp"
+#include "../solvers/CFEASolver.hpp"
+#include "../solvers/CTemplateSolver.hpp"
+#include "../solvers/CDiscAdjSolver.hpp"
+#include "../solvers/CDiscAdjFEASolver.hpp"
+#include "../solvers/CFEM_DG_EulerSolver.hpp"
+#include "../solvers/CFEM_DG_NSSolver.hpp"


These should go to the cpp otherwise a change to any solver will trigger recompilation of all drivers.

Oooo, that's a good point. Will move these

pcarruscag · 2020-01-26T14:16:43Z

SU2_CFD/include/solvers/CSolver.hpp

+  inline CVariable* GetNodes() const {
+    assert(base_nodes!=nullptr && "CSolver::base_nodes was not set properly, see brief for CSolver::SetBaseClassPointerToNodes()");
+    return base_nodes;
+  }


Methods returning non-const pointers to class members should not be declared const IMO. While the method itself does not change the state of the object, it still provides a way to (changing the nodes in this case), this would allow a function taking a const CSolver* to change its state via such methods which would be misleading.
So, until we can return const CVariable*, this should not be const.

Fair enough, I agree.

jayantmukho added 15 commits December 19, 2019 14:48

split hpp files up

3702fbe

split the header files completely

0d82d4e

split the header files completely

50a5928

split into inidividual inline files for solvers

026f227

added .inl includes in the header files

c94466d

.cpp .hpp and .inl files are seperated and it builds!

df79c41

fixing typo in meson build

8cbac15

added some filenames to the meson.build files

03b03cf

removed old solver.cpp files

8bab2b5

removing old solver header files

a073a13

testing splitting scripts

8b9cbdc

added inline functions to .hpp files

aa673d7

removed references to solver inline files

21692ad

fix some of the inline functions

0f44cb2

removed solver.inl files

6c216c9

pr-triage bot added the PR: unreviewed label Jan 11, 2020

pcarruscag approved these changes Jan 13, 2020

View reviewed changes

SU2_DOT/src/meson.build Outdated Show resolved Hide resolved

SU2_CFD/include/SU2_CFD.hpp Outdated Show resolved Hide resolved

pr-triage bot added PR: reviewed-approved and removed PR: unreviewed labels Jan 13, 2020

jayantmukho added 2 commits January 13, 2020 10:55

fixing indenting and removing some includes

2c13bbd

more indentation fixes

92d3776

pr-triage bot added PR: unreviewed and removed PR: reviewed-approved labels Jan 13, 2020

jayantmukho added 2 commits January 13, 2020 11:04

Merge remote-tracking branch 'origin/develop' into restructure_solvers

8a61bde

more indentation fixes

55103ac

jayantmukho added 2 commits January 14, 2020 08:49

merged with develop

c0da336

trimmed tabs and whitespaces

d2fa8c1

WallyMaier removed the PR: unreviewed label Jan 14, 2020

pcarruscag and others added 3 commits January 22, 2020 16:43

add cli option for MPI_THREAD_MULTIPLE

63d4be8

two thirds of the way done with virtual specifiers

185547a

adding const to fix segfault on transonic cases

89f2dcd

pcarruscag reviewed Jan 23, 2020

View reviewed changes

SU2_CFD/include/solvers/CAdjTurbSolver.hpp Show resolved Hide resolved

pcarruscag and others added 5 commits January 23, 2020 13:50

Merge branch 'hybrid_parallel_fea_solver' into feature_hybrid_paralle…

bdc6a17

…l_and_SIMD_tmp

Merge remote-tracking branch 'upstream/restructure_solvers' into feat…

016860d

…ure_hybrid_parallel_and_SIMD_tmp

finished the virtual and const specifications

3e31a20

declaring classes as final

4df3980

minor fixes

b29fb48

jayantmukho commented Jan 23, 2020

View reviewed changes

pcarruscag and others added 5 commits January 24, 2020 14:28

Merge remote-tracking branch 'upstream/restructure_solvers' into feat…

a82d0cb

…ure_hybrid_parallel_and_SIMD_tmp

merge

ad5a4a7

Merge remote-tracking branch 'upstream/develop' into feature_hybrid_p…

ba9e218

…arallel_and_SIMD_tmp

remove dirichlet and neumann BCs:

7642268

merged with develop

ad84b85

jayantmukho added 2 commits January 24, 2020 12:37

formatting of CFEASolver and CMeshSolver

8cd8260

getting rid of some compiler warnings

ff0f049

jayantmukho commented Jan 24, 2020

View reviewed changes

pcarruscag reviewed Jan 26, 2020

View reviewed changes

jayantmukho added 2 commits January 26, 2020 23:19

moved some includes, added some overrides, removed a const

f8e862f

reverting parentheses fix in CConjugateHeatInterface

eff6d67

jayantmukho added the changelog:none label Jan 27, 2020

jayantmukho merged commit 1a64163 into develop Jan 27, 2020

jayantmukho deleted the restructure_solvers branch January 27, 2020 17:49

pr-triage bot added PR: merged and removed PR: unreviewed labels Jan 27, 2020

talbring added changelog:chore and removed changelog:none labels Jan 31, 2020

Restructuring of solver files #849

Restructuring of solver files #849

Uh oh!

Conversation

jayantmukho commented Jan 11, 2020

Proposed Changes

Related Work

PR Checklist

Uh oh!

pcarruscag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jayantmukho commented Jan 13, 2020

Uh oh!

talbring commented Jan 14, 2020

Uh oh!

Uh oh!

jayantmukho 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!

Uh oh!

jayantmukho commented Jan 24, 2020

Uh oh!

jayantmukho 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!

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 Jan 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pcarruscag Jan 26, 2020 •

edited

Loading