Numerics refactoring for CAvgGrad- and CUpwSca- classes #422

clarkpede · 2017-07-31T20:46:49Z

I noticed there was a lot of duplicate code in the turbulent numerics files (numerics_structure.hpp and numerics_direct_turbulent.cpp). Since I'm working with new model equations for hybrid RANS/LES, I didn't want to add to the technical debt. So I decided to refactor the turbulence numerics using the template design pattern.

Basically, I moved common behavior for the viscous and the upwinding residuals to respective base classes and then left the specifics of each model to derived classes. I did my best to not change any of the underlying behavior.

The source residual calculations for SST and SA have little in common. I did not apply the template design pattern to them.

I named the template classes CUpwSca_Abstract and CAvgGrad_Abstract because the _Template suffix was already taken.

I also noticed that there were almost no difference between the old CAvgGrad_ classes and the CAvgGradCorrected_ classes. The gradient correction is such a small change that it's better to have a single class with a switch than two classes that are about 95% similar. So I added a boolean property and logical switching if correction is to be applied to the gradients. This boolean parameter is now a parameter for the constructor.

Overall, these changes trim down a lot of duplicate code (40% of the lines in numerics_direct_turbulent.cpp). These changes could be extended to other areas (such as the transition model), but I left that to future pull requests.

…dient.

ghost

Hi @clarkpede, thanks a lot for contributing to the SU2 code. Looking at your summary it seems to be a great addition to SU2 that we should extend to other parts of the software. I'm going to expend some time reviewing your source code and I'll let you know if there is something that we need to work on.

Thanks a lot for your time an generosity!

Francisco

ghost · 2017-08-06T06:06:41Z

SU2_CFD/include/numerics_structure.hpp

@@ -66,13 +66,13 @@ class CNumerics {
  su2double *Enthalpy_formation;
  su2double Prandtl_Lam;        /*!< \brief Laminar Prandtl's number. */
  su2double Prandtl_Turb;    /*!< \brief Turbulent Prandtl's number. */
-


Thanks for fixing these extra spaces

I don't think that this is intended. Usually in some editors the default is to remove empty lines when saving the file. In fact, I would suggest to revert these changes as they give the potential to lead to conflicts in the future. We should be consistent regarding the handling of white spaces and empty lines.

I just reverted the unnecessary whitespace changes. I agree that they should be eliminated to reduce the number of "false conflicts," and I also agree that SU2 could benefit from more consistent use of whitespace.

ghost · 2017-08-06T06:12:12Z

SU2_CFD/include/numerics_structure.hpp

@@ -2071,34 +2071,64 @@ class CUpwLin_AdjTurb : public CNumerics {
 };

 /*!
- * \class CUpwSca_TurbSA
- * \brief Class for doing a scalar upwind solver for the Spalar-Allmaral turbulence model equations.
+ * \class CUpwSca_Abstract


Makes a lot of sense to me. By the way, we are using a very basic linear upwind scheme based on the velocity. Have you ever tried to use a more elaborate upwind? (e.g. something similar to the upwind scheme that we use for the mass conservation equation like using Roe's averaged variables).

No, I haven't looked into using the higher order upwinding in the turbulence classes. For the most part, our group has had other priorities (like implementing a higher order time-stepping).

ghost · 2017-08-06T06:17:06Z

SU2_CFD/include/numerics_structure.hpp

- * \class CUpwSca_TurbML
- * \brief Class for doing a scalar upwind solver for the Spalar-Allmaral turbulence model equations.
+ * \class CUpwSca_TurbSA
+ * \brief Class for doing a scalar upwind solver for the Spalar-Allmaras turbulence model equations.


Thanks for fixing the typo in Allmaras!

ghost · 2017-08-06T06:21:24Z

SU2_CFD/include/numerics_structure.hpp

 * \ingroup ConvDiscr
 * \author A. Bueno.
 * \version 5.0.0 "Raven"
 */
-class CUpwSca_TurbML : public CNumerics {
+class CUpwSca_TurbSA : public CUpwSca_Abstract {


This is a good opportunity to fix this problem. If you have a while could you please eliminate all classes and subroutines with "TurbML": CUpwSca_TurbML, CAvgGrad_TurbML, CAvgGradCorrected_TurbML, CTurbMLVariable. ML stands for Machine Learning, that was an old implementation that was eliminated from the main SU2 distribution... but it seems that there are still some residuals. Thanks!

I had no idea what those classes were for. Thanks for the clarification. I'll make that change one of my next commits.

Thanks for removing the TurbML stuff!

ghost · 2017-08-06T06:24:56Z

SU2_CFD/src/driver_structure.cpp

-      if (spalart_allmaras) numerics_container[iMGlevel][TURB_SOL][VISC_TERM] = new CAvgGradCorrected_TurbSA(nDim, nVar_Turb, config);
-      else if (neg_spalart_allmaras) numerics_container[iMGlevel][TURB_SOL][VISC_TERM] = new CAvgGradCorrected_TurbSA_Neg(nDim, nVar_Turb, config);
-      else if (menter_sst) numerics_container[iMGlevel][TURB_SOL][VISC_TERM] = new CAvgGradCorrected_TurbSST(nDim, nVar_Turb, constants, config);
+      if (spalart_allmaras) numerics_container[iMGlevel][TURB_SOL][VISC_TERM] = new CAvgGrad_TurbSA(nDim, nVar_Turb, true, config);


Thanks for simplifying the structure. And yes, a boolean works better... in fact, in the future we should probably only use the corrected version which is quite standard.

ghost

I think it is a very useful contribution, and an example for future implementations. I only have two questions related with the naming convention:

What is the subroutine "FinishResidualCalc" is it a new name to call "ComputeResidual" if that is the case, for consistency with the rest of the code we should probably use "ComputeResidual"
You are adding the prefix "_Abstract" to the new parent classes. For the time being, as we are applying this only for the turbulence model, would it be possible to add also the key word "Turb". And... are you completely sure about using the word "Abstract" does it makes sense to use something like for example CAvgGrad_TurbBaseline instead of CAvgGrad_Abstract. What do you think?

Best,
Francisco

clarkpede · 2017-08-07T15:24:22Z

In reply to:

What is the subroutine "FinishResidualCalc" is it a new name to call "ComputeResidual" if that is the case, for consistency with the rest of the code we should probably use "ComputeResidual"

I would have liked to have named it the way you suggested. But unfortunately, it's not possible. There's two reasons for this: one is more philosophical, the other is due to a programming restriction.

The real residual calculation is a larger method in the parent class (i.e. CAvgGrad_Abstract::ComputeResidual). The method currently called FinishResidualCalc is just a step in that larger process.
Since the real residual calculation is in the parent class, we need to be able to call it from derived classes (i.e. CAvgGrad_TurbSA::ComputeResidual). If we also name a separate method in the derived class ComputeResidual, then that method now overrides the method in the parent class. Instead of performing the whole calculation, callers would only get the final step in the calculation.

I'm not fixed on the name. Honestly, I picked it somewhat arbitrarily. I just know that "ComputeResidual" isn't an option. What do you suggest as a better name?

clarkpede · 2017-08-07T15:45:45Z

In reply to:

You are adding the prefix "_Abstract" to the new parent classes. For the time being, as we are applying this only for the turbulence model, would it be possible to add also the key word "Turb". And... are you completely sure about using the word "Abstract" does it makes sense to use something like for example CAvgGrad_TurbBaseline instead of CAvgGrad_Abstract. What do you think?

I'm not fixed on "Abstract". In fact, I wanted to use the suffix "Template" instead, and only decided on "Abstract" as a less satisfactory second choice. As the code currently stands, I totally agree with you. "Turb" would be a better suffix. But I was also thinking about possible changes in the future.

I can see these classes (CAvgGrad_Abstract and CUpwSca_Abstract) being used for the transition model (in numerics_direct_transition.cpp) as well. Do you agree? Do you think this refactoring should be applied to the transition model?

I didn't picture the transition model as part of this pull request's scope. I wanted to include the transition model as a separate pull request. If we do want to refactor the transition model to eliminate the duplicate code there, then I don't think "Turb" or "TurbBaseline" are good suffixes.

If you do want the duplicate code in the transition model eliminated, then which would you prefer: "Baseline", "Abstract", "Outline", or "Scalar"? Or do you have another idea?

…ring

talbring

Thank you @clarkpede for these changes. That really simplifies the turbulent numerics and could be also be done for the other classes in the future.

I just have some minor comments below.

talbring · 2017-08-10T09:08:59Z

SU2_CFD/include/numerics_structure.hpp

@@ -66,13 +66,13 @@ class CNumerics {
  su2double *Enthalpy_formation;
  su2double Prandtl_Lam;        /*!< \brief Laminar Prandtl's number. */
  su2double Prandtl_Turb;    /*!< \brief Turbulent Prandtl's number. */
-


I don't think that this is intended. Usually in some editors the default is to remove empty lines when saving the file. In fact, I would suggest to revert these changes as they give the potential to lead to conflicts in the future. We should be consistent regarding the handling of white spaces and empty lines.

talbring · 2017-08-10T09:29:56Z

SU2_CFD/include/numerics_structure.hpp

-  su2double **Mean_GradTurbVar;
-  su2double *Proj_Mean_GradTurbVar_Kappa, *Proj_Mean_GradTurbVar_Edge;
-  su2double *Edge_Vector;
+class CAvgGrad_Abstract : public CNumerics {


I would suggest to use a different name for this class. Simply CAvgGrad_Turb would be ok I think.

Did you get a chance to look at my earlier reply to Dr. Palacios' similar comment? What are your thoughts? Should we put the suffix as "Turb", and change it in the future, or have the name be more general, such as "Baseline", "Abstract", "Outline", or "Scalar"?

Oops sorry. Didn't read that.

economon · 2017-08-11T05:06:11Z

@clarkpede, @talbring, @fpalacios: I think this is a great discussion, and a good opportunity to think about future expansion of the code with this naming (the changes to other classes could come later). I envision that we will use the same high-level conv/visc schemes abstracted here for the transition model but also for general scalar transport equations in the future, such as passive scalars, combustion models, etc. I am fine with "Turb" but "Scalar" could be the most general.

talbring · 2017-08-11T09:43:57Z

I agree that if we plan to have it even more general in the future, then we should probably go with the term "Scalar".

ghost · 2017-08-13T21:40:09Z

Yep, "Scalar" sounds great. As for me, just implement that small change and I would very happy with your implementation.

Thanks again,
Best,
Francisco

…ring

… git conflicts

clarkpede · 2017-08-20T02:23:19Z

All right. I changed the suffixes from "_Abstract" to "_Scalar". I think that resolves all previous issues.

I noticed Codacy flagged some code quality issues that predate this pull request. I can change those in a separate pull request, unless any of you want me to do that here.

economon

Thanks for this PR that cleans up redundant code - this type of thing keeps with the C++ philosophy of code reuse, and we should encourage as much of this as possible. Nice work!

Just one comment in the review, and don't worry about Codacy. We are just trying it out at the moment.

economon · 2017-08-21T02:54:40Z

SU2_CFD/include/numerics_structure.hpp

 * \version 5.0.0 "Raven"
 */
-class CUpwSca_TurbSA : public CNumerics {
+class CUpwSca_Scalar : public CNumerics {


For me, CUpwSca_Scalar is a bit redundant... CUpw_Scalar or CUpwScalar might be better. Perhaps the latter is best, as later child classes can be more specific to the model, e.g., CUpwScalar_TurbSA, etc.

…ring

ghost · 2017-09-03T00:55:48Z

Hi @talbring are you happy with this version of the pull request? Thanks!

talbring · 2017-09-03T20:19:36Z

Looks good now ! Thanks !

economon · 2017-09-05T21:39:23Z

Looks good to me too now, thanks for the changes, merging...

clarkpede and others added 13 commits July 28, 2017 13:00

Added a template method to the CAvgGrad numerics classes

50712ff

Fixed bugs in member initialization.

ea066a4

Removed static member in-class definitions, for backwards compatibility

d4cafa2

Added template method for scalar upwinding.

ea2164c

Merged the viscous and corrected viscous numerics classes

2b99bd2

Merged in recent changes from develop

e797a3a

Fixed some small bugs in the templated methods

5becd48

Bug fixed; viscous residual previously using uncorrected velocity gra…

62b875b

…dient.

Switch travis build to O1 optimization to reduce test runtime

2dc81de

Removed tests from Travis just to get tests in under max runtime

d087d2f

Renaming classes and updating documentation

e0559e1

Reverting .travis.yml to default for develop branch

dfbc9e6

Reverting parallel and serial regression tests to develop branch

245cdee

clarkpede changed the title ~~Numerics refactoring~~ Numerics refactoring for CAvgGrad- and CUpwSca- classes Jul 31, 2017

Merge branch 'develop' into numerics-refactoring

8d28766

ghost reviewed Aug 6, 2017

View reviewed changes

ghost suggested changes Aug 6, 2017

View reviewed changes

clarkpede added 2 commits August 7, 2017 11:05

Removed old TurbML variables and numerics

e72be9b

Merge remote-tracking branch 'upstream/develop' into numerics-refacto…

01c6604

…ring

talbring requested changes Aug 10, 2017

View reviewed changes

ghost approved these changes Aug 13, 2017

View reviewed changes

clarkpede added 3 commits August 19, 2017 19:03

Renamed template classes from _Abstract to _Scalar

dd9bff1

Merge remote-tracking branch 'upstream/develop' into numerics-refacto…

e79c2e3

…ring

Reverting changes in whitespace per talbring's request to avoid false…

9bb8612

… git conflicts

Merge branch 'develop' into numerics-refactoring

307f681

economon reviewed Aug 21, 2017

View reviewed changes

economon and others added 4 commits August 20, 2017 19:56

Merge branch 'develop' into numerics-refactoring

5918ced

Changed naming of CUpwSca_Scalar to CUpwScalar

50f4adb

Merge remote-tracking branch 'upstream/develop' into numerics-refacto…

669d7b2

…ring

Merge branch 'develop' into numerics-refactoring

8eb4a88

talbring approved these changes Sep 3, 2017

View reviewed changes

Merge branch 'develop' into numerics-refactoring

008f0e5

economon merged commit c3993f3 into su2code:develop Sep 5, 2017

clarkpede deleted the numerics-refactoring branch May 17, 2018 17:53

This was referenced Jun 24, 2019

Limiter related fixes #718

Merged

Possible correctness issue in how turbulence equations are discretized #721

Closed

clarkpede mentioned this pull request Sep 25, 2019

WIP General Framework for Scalar Transport Equations #777

Closed

4 tasks

Numerics refactoring for CAvgGrad- and CUpwSca- classes #422

Numerics refactoring for CAvgGrad- and CUpwSca- classes #422

Uh oh!

Conversation

clarkpede commented Jul 31, 2017

Uh oh!

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

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

clarkpede commented Aug 7, 2017

Uh oh!

clarkpede commented Aug 7, 2017

Uh oh!

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

economon commented Aug 11, 2017

Uh oh!

talbring commented Aug 11, 2017

Uh oh!

ghost commented Aug 13, 2017

Uh oh!

clarkpede commented Aug 20, 2017

Uh oh!

economon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 3, 2017

Uh oh!

talbring commented Sep 3, 2017

Uh oh!

economon commented Sep 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants