Solver Factory class #881

talbring · 2020-02-17T18:18:46Z

Proposed Changes

In order to structure the construction of the solvers a little bit and to remove the dependency of the driver on the solver, I created a static solver factory class. There are routines to create individual instances as well as the whole container. It should be much easier now to see what position in the containers are actually allocated for the different solvers.

The createSolver routine is a generic one that simply calls new. The others have some additional checks or calls for the construction. The idea here was to remove any if statements in the createSolverContainer routine to ease readability.

If everybody agrees, we could use a similar structure for other classes as well. Let me know what you think.

Note from TobiKattmann: This PR depends on #861

Related Work

Resolve any issues (bug fix or feature request), note any related PRs, or mention interactions with the work of others, if any.

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 · 2020-02-17T18:47:31Z

LGTM what other families to you have in mind? I think doing this for the numerics would be very nice but they should move to the solver.

SU2_CFD/src/solvers/CNSSolver.cpp

talbring · 2020-02-18T12:39:44Z

Since I am already at it, I am gonna add factory classes for numerics (we can use them even if we move the allocation to the solvers) and integration.

economon

LGTM. The factory pattern is a good one that we should have been using for a long time. I am for adding this for the integration / numerics as well.

economon · 2020-02-18T16:10:48Z

SU2_CFD/src/drivers/CMultizoneDriver.cpp


    for (unsigned short iSol = 0; iSol < MAX_SOLS; iSol++){
-      if (solver_container[iZone][INST_0][MESH_0][iSol] != NULL){
+      if (solver_container[iZone][INST_0][MESH_0][iSol] != nullptr){


I like the switch to nullptr

economon · 2020-02-18T16:11:13Z

SU2_CFD/src/drivers/CDriver.cpp

-#include "../../include/solvers/CIncEulerSolver.hpp"
-#include "../../include/solvers/CNSSolver.hpp"
-#include "../../include/solvers/CIncNSSolver.hpp"
-#include "../../include/solvers/CTurbSASolver.hpp"


Getting all of the headers out of CDriver is great 👍

SU2_CFD/include/solvers/CSolverFactory.hpp

TobiKattmann · 2020-02-25T08:51:40Z

SU2_CFD/src/drivers/CDriver.cpp

 void CDriver::Solver_Preprocessing(CConfig* config, CGeometry** geometry, CSolver ***&solver) {
-
-  unsigned short iSol;
+      


well well, what do we have here 🕵 (Detective Whitespace on the hunt)

SU2_CFD/src/drivers/CDriver.cpp

TobiKattmann · 2020-02-25T09:10:56Z

SU2_CFD/src/drivers/CDriver.cpp

-
  delete [] solver[val_iInst];
-
+  


🕵 This is one of the easier cases for Detective Whitespace

SU2_CFD/src/solvers/CSolverFactory.cpp

…e/SU2 into solver_factory

SU2_CFD/src/integration/CIntegrationFactory.cpp

SU2_CFD/include/solvers/CSolverFactory.hpp

TobiKattmann · 2020-02-28T13:32:22Z

SU2_CFD/include/integration/CIntegrationFactory.hpp

+   * \param kindSubSolver    - The kind of sub solver
+   * \return                 - Pointer to the allocated integration instance
+   */
+  static CIntegration* createIntegration(INTEGRATION_TYPE integrationType);


integrationType instead of kindSubSolver (although integrationType directly depends on kindSubSolver as far as I understand)

TobiKattmann · 2020-02-28T13:51:00Z

SU2_CFD/src/drivers/CDriver.cpp

    case TEMPLATE_SOLVER: template_solver = true; break;
    case EULER : case INC_EULER: euler = true; break;
    case NAVIER_STOKES: case INC_NAVIER_STOKES: ns = true; break;
    case RANS : case INC_RANS: ns = true; turbulent = true; break;


Could we remove the functions Solver/Integration_Preprocessing (and all the *_Preprocessing routines that follow) and pull the few lines up to the CDriver constructor as it is only called from there.
This function has 4 lines of code and only hands work over to the factory 🏭 (why didn't I think earlier of using the factory emoji 🤦‍♂ )

I will leave it like that for the moment. We can consider a clean-up once we have factory methods for the rest of the containers (especially numerics).

SU2_CFD/src/solvers/CEulerSolver.cpp

SU2_CFD/include/integration/CIntegrationFactory.hpp

…actory

TobiKattmann · 2020-03-02T13:41:07Z

SU2_CFD/include/output/COutputFactory.hpp

+   * \param[in] kindSolver       - The kind of main solver
+   * \return                     - Pointer to the allocated Output
+   */
+  static COutput* createOutput(ENUM_MAIN_SOLVER kindSolver, CConfig *config, int nDim);


missing param[in]

pcarruscag · 2020-03-02T14:16:05Z

SU2_CFD/include/output/COutputFactory.hpp

+  /*!
+   * \brief Private constructor to avoid creating instances of this class
+   */
+  COutputFactory();


Thanks for the const stuff, everything looks very nice.
I think it is a good idea to have a separate numerics PR.

Final note, the preferred C++11 way to achieve this is to delete the default constructor instead of making it private. COutputFactory() = delete;

Perfect, thanks! If that's the preferred way then we should do it 👍

At least according to Scott Meyers xD

TobiKattmann · 2020-03-02T15:39:04Z

As all comments are addressed, I am merging this in. Thanks @talbring for the effort 💐

talbring added 6 commits February 17, 2020 16:33

Adding CSolverFactory class

baa8f69

Adapting CDriver

836fb97

Updating build files

f5c63dc

Adding flow solver factory routine

79fcb8b

Simplified solver postprocessing

7cf3fec

Added generic factory routine

604fe1f

pcarruscag reviewed Feb 17, 2020

View reviewed changes

SU2_CFD/src/solvers/CNSSolver.cpp Outdated Show resolved Hide resolved

talbring added 2 commits February 18, 2020 13:37

Fixing heat solver allocation

819e20e

Merge branch 'develop' of github.com:su2code/SU2 into solver_factory

6d5aff1

economon approved these changes Feb 18, 2020

View reviewed changes

pr-triage bot added the PR: reviewed-approved label Feb 18, 2020

TobiKattmann changed the title ~~Solver Factory class~~ WIP Solver Factory class Feb 25, 2020

TobiKattmann requested changes Feb 25, 2020

View reviewed changes

pr-triage bot removed the PR: reviewed-approved label Feb 25, 2020

talbring added 4 commits February 26, 2020 11:34

Cleaning whitespaces and adding missing file header

ab099ac

Adding missing doxygen comment

2008f65

Merge branch 'hybrid_parallel_compressible_rans' of github.com:su2cod…

1f59070

…e/SU2 into solver_factory

Some bugfixes

208330a

talbring mentioned this pull request Feb 26, 2020

Hybrid Parallel Compressible RANS Solvers #861

Merged

5 tasks

dep bot added the dependent label Feb 26, 2020

talbring added 6 commits February 26, 2020 16:38

Merge branch 'develop' of github.com:su2code/SU2 into solver_factory

9a17201

Adapting bars SST test case

ccb3ea6

Renaming solver enum

ce2a9ac

refactoring of solver factory header and introduction of sub solver enum

3046077

refactoring of solver factory source

bb3cd48

Introducing factory class for integration structure

b7f5eae

pcarruscag reviewed Feb 27, 2020

View reviewed changes

SU2_CFD/src/integration/CIntegrationFactory.cpp Outdated Show resolved Hide resolved

adding private constructor to factory class

3bcd6ab

talbring added 5 commits February 28, 2020 10:44

Adding dofs computation

cdf5aa8

Small fix for disc adj heat solver

3953f68

adding solver meta data

767d87f

some clean up of integration classes

ba7641b

Small fix

a42c86e

TobiKattmann removed the dependent label Feb 28, 2020

talbring added 2 commits February 28, 2020 14:42

Add radiation solver

7cdddf0

Add missing comment

3e3a20b

pcarruscag reviewed Feb 28, 2020

View reviewed changes

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

TobiKattmann reviewed Feb 28, 2020

View reviewed changes

pcarruscag reviewed Feb 28, 2020

View reviewed changes

SU2_CFD/src/solvers/CEulerSolver.cpp Outdated Show resolved Hide resolved

Add missing comment and missing break

b7f712d

pcarruscag reviewed Feb 28, 2020

View reviewed changes

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

talbring added 4 commits March 2, 2020 09:40

Merge branch 'develop' of github.com:su2code/SU2 into solver_factory

b7806b8

Merge branch 'solver_factory' of github.com:su2code/SU2 into solver_f…

a32750a

…actory

increased MAX_SOLS

2fb1bdf

adding output factory

fc1e70a

TobiKattmann approved these changes Mar 2, 2020

View reviewed changes

talbring added 2 commits March 2, 2020 14:47

Fixed version number

977a801

Fix whitespace and const

dfd59f9

talbring changed the title ~~WIP Solver Factory class~~ Solver Factory class Mar 2, 2020

dep bot added the dependent label Mar 2, 2020

talbring added changelog:none and removed dependent labels Mar 2, 2020

pcarruscag approved these changes Mar 2, 2020

View reviewed changes

Changed private constructor to deleted

e50714c

TobiKattmann merged commit f42191d into develop Mar 2, 2020

TobiKattmann deleted the solver_factory branch March 2, 2020 15:39

pr-triage bot added the PR: merged label Mar 2, 2020

		void CDriver::Solver_Preprocessing(CConfig* config, CGeometry geometry, CSolver *&solver) {

		unsigned short iSol;

Solver Factory class #881

Solver Factory class #881

Uh oh!

Conversation

talbring commented Feb 17, 2020 • edited by TobiKattmann Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Related Work

PR Checklist

Uh oh!

pcarruscag commented Feb 17, 2020

Uh oh!

Uh oh!

talbring commented Feb 18, 2020

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcarruscag Mar 2, 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!

Choose a reason for hiding this comment

Uh oh!

TobiKattmann commented Mar 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

talbring commented Feb 17, 2020 •

edited by TobiKattmann

Loading

pcarruscag Mar 2, 2020 •

edited

Loading