WIP General Framework for Scalar Transport Equations #777

economon · 2019-08-27T22:22:27Z

Proposed Changes

Give a brief overview of your contribution here in a few sentences.

This PR adds the necessary base classes (solver, numerics, variable) for having an arbitrary number of scalar transport equations added to flow problems in a segregated manner (alongside a turbulence model, if necessary). Most of the numerics is shared with the scalar methods for the turb. solvers to eliminate duplication, and it should be easy now to add child classes for any new scalar transport models.

In this PR, only a passive scalar has been implemented, but we are also building on this framework to add a flamelet model for combustion as a child class. A passive scalar test case will be added.

It still needs more work, but it is important to have the skeleton merged now to lessen the burden of fixing conflicts with the ongoing restructuring in driver, variable, output, etc.

Related Work

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

N/A

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.

…bustion model. Needs testing.

pcarruscag

I like the feature and the implementation is clean. I do not like the architecture, something like this should be the base of the turbulence and heat solvers.

pcarruscag · 2019-08-28T08:04:31Z

SU2_CFD/include/SU2_CFD.hpp


 #include "../../Common/include/mpi_structure.hpp"
-#include <CLI11.hpp>
+#include "../../externals/CLI11/CLI11.hpp"


Is this what we do for other externals or is adding to the include path more appropriate?

pcarruscag · 2019-08-28T08:13:42Z

SU2_CFD/include/variables/CIncNSVariable.hpp

   * \brief Set all the primitive variables for incompressible flows
   */
-  bool SetPrimVar(su2double eddy_visc, su2double turb_ke, CFluidModel *FluidModel);
+  bool SetPrimVar(su2double eddy_visc, su2double turb_ke, su2double *scalar, CFluidModel *FluidModel);


Not applicable to Euler?

pcarruscag · 2019-08-28T08:28:22Z

SU2_CFD/src/solver_direct_scalar.cpp

+
+  bool turbulent = ((config->GetKind_Solver() == RANS) ||
+  (config->GetKind_Solver() == DISC_ADJ_RANS));
+  bool turb_SST  = ((turbulent) && (config->GetKind_Turb_Model() == SST));


Are you missing SST_SUS now?

pcarruscag · 2019-08-28T08:29:12Z

SU2_CFD/src/solver_direct_scalar.cpp

+                                           CSolver **solver_container,
+                                           CNumerics *numerics,
+                                           CNumerics *second_numerics,
+                                    CConfig *config,


Indentation

pcarruscag · 2019-08-28T08:29:52Z

SU2_CFD/src/solver_direct_scalar.cpp

+      /*--- Mass diffusivity coefficients. ---*/
+
+      second_numerics->SetDiffusionCoeff(node[iPoint]->GetDiffusivity(),
+                                  NULL);


More indentation

pcarruscag · 2019-08-28T08:41:25Z

SU2_CFD/include/solver_structure.hpp

+   */
+
+  void Upwind_Residual(CGeometry *geometry,
+                       CSolver **solver_container,


Are we supposed to shorten the names? Otherwise we start having inconsistencies between driver, solver, etc.

pcarruscag · 2019-08-28T08:46:20Z

SU2_CFD/src/output_structure.cpp

+  if (scalar)
+    switch (config->GetKind_Scalar_Model()) {
+      case PASSIVE_SCALAR: case PROGRESS_VARIABLE:
+        nVar_Scalar = 1;


Hardcoded nVar? How does one add more scalar variables?

pcarruscag · 2019-08-28T08:48:37Z

SU2_CFD/src/solver_direct_scalar.cpp

+
+      /*--- Mean flow primitive variables using gradient reconstruction and limiters ---*/
+
+      Gradient_i = solver_container[FLOW_SOL]->node[iPoint]->GetGradient_Primitive();


This is what the turbulent solver does almost verbatim. Are you moving the heat and turbulence solvers to this framework?

This should be possible without too much effort... actually this is already done for the convective schemes. The two next targets are the viscous scheme in numerics (one more base class) and then both the turb and heat solvers can inherit from the base scalar solver class in order to remove some duplication (like viscous residual above) - needs to be done with some care

This is great work. I'm glad to see you've made my base classes from the turbulence solver even more general. Edit: I just noticed you said you're doing it for the viscous numerics as well. Looks great!

Great, it would be nice to re-use the muscl reconstruction bit of the code done in UpwindResidual too.

I want to change my mind, can you keep the solvers as is for now? (merging #753 is triple the work otherwise...)

Should I avoid just the solvers? Does it cause an issue to adjust the viscous numerics?

I would like to clean up some things for this PR and hook up the new output classes for the scalars too if that won't cause any problems for you either.

Numerics is ok to change, selfish request but if the turbulence and heat solvers are changed significantly, I would have to re-apply the changes to them, unless you do that work already in the framework of #753 :p

I have now updated the classes so that all of the turbulent convective, viscous, and variable classes inherit from a scalar base class versions. This removes some duplicate code. I have not yet adjusted the solver classes, since this is a little more delicate.

Btw, this PR has now also been updated with the new contiguous variable structures and the new output.

pcarruscag · 2019-08-28T08:53:55Z

SU2_CFD/src/solver_direct_scalar.cpp

+  bool turb_SST  = ((turbulent) && (config->GetKind_Turb_Model() == SST));
+  bool turb_SA   = ((turbulent) && (config->GetKind_Turb_Model() == SA));
+
+  /*--- Dimension of the problem --> passive scalar will only ever


Any particular reason for only 1 variable?

pcarruscag · 2019-08-28T08:55:13Z

SU2_CFD/src/solver_direct_scalar.cpp

+  lowerlimit = new su2double[nVar];
+  upperlimit = new su2double[nVar];
+
+  lowerlimit[0] = -1.0e15;


not from config?

oleburghardt

Hi Tom, this looks good to me, just a couple of questions below.

I'm sure you've already talked about including the new output structure in this PR. Is this the plan then?

Another concern of mine is how to proceed with all the weakly_coupled_heat-related lines. As you know, by adding the heat equation to the incompressible solver they became obsolete and I thought one should just leave them there in order to replace them by a general scalar model one day. Now we have them side by side.
Though nothing's wrong with keeping them anyway, I'm just interested in your view on that (see related comment on flow solver positions).

oleburghardt · 2019-09-25T10:46:21Z

SU2_CFD/include/variables/CScalarVariable.hpp

+   */
+  inline void SetDiffusivity(su2double      val_diffusivity,
+                             unsigned short val_ivar) {
+    Diffusivity[val_ivar] = val_diffusivity;


It comes handy that the scalar transport framework is based on (mass) diffusivity. That way it works very similar to the heat solver which is also based on (thermal) diffusivity and one could easily make it a child class.

oleburghardt · 2019-09-25T12:14:56Z

SU2_CFD/src/numerics_direct_scalar.cpp

+
+}
+
+void CAvgGradScalar_General::ExtraADPreaccIn() { }


Should the preaccumulation really be wrapped? I don't find them too disturbing on the ComputeResidual-level. (I'd even prefer to have them there because errors might be more apparent then for the reader.)
It's also not entirely consistent so far, e.g. here or for grid velocity.

I can chime in here. In the original implementation, the "scalars" were the turbulent variables. I was the one who applied the template design pattern to the AvgGrad classes in PR #422.

The motivation was this: In the template method, you find what's the same and what's different between similar functions. You put everything that's the same into a base class (e.g. CAvgGradScalar), then put everything that differs into functions implemented by the derived class. So some SetPreaccIn calls are common, and they're in the base class (CAvgGradScalar). Those that are unique are included in the ExtraADPreaccIn. That way the CAvgGradScalar::ComputeResidual just does this for all child classes:

void CAvgGradScalar::ComputeResidual(su2double *val_residual, su2double **Jacobian_i, su2double **Jacobian_j, CConfig *config) { AD::StartPreacc(); AD::SetPreaccIn(Coord_i, nDim); AD::SetPreaccIn(Coord_j, nDim); AD::SetPreaccIn(Normal, nDim); AD::SetPreaccIn(ScalarVar_Grad_i, nVar, nDim); AD::SetPreaccIn(ScalarVar_Grad_j, nVar, nDim); AD::SetPreaccIn(Diffusion_Coeff_i, nVar); AD::SetPreaccIn(Diffusion_Coeff_j, nVar); if (correct_gradient) { AD::SetPreaccIn(ScalarVar_i, nVar); AD::SetPreaccIn(ScalarVar_j, nVar); } ExtraADPreaccIn();

Now, if you're OK with code duplication, then all the SetPreaccIn calls could be made in derived classes. But it's impossible to put them all in the ComputeResidual class because not all derived classes use the same variables.

I see (I've overseen the _General extension to spot the overload), thanks for the explanation! I didn't notice the new templated structure..
Problems due to erroneous preaccumulation happened quite often to me (and they are very hard to track down), so that's why I'm so worried about those lines :D - though it's not reason enough for duplicating the code.

If you missed it, then I would recommend that @economon add some explanatory comments about CAvgGradScalar_General either in the header file or the cpp file. That way future code readers/programmers don't have the same confusion.

Unless @economon would prefer that I do the comments.

Thanks for highlighting this topic. I think @oleburghardt's experience is very common. I had the same issue: the adjoint was not working with scalars for a while before I realized I had made the same mistake of not adding the diffusivity to the preaccumulation. This is very easy to overlook.

Is there a change we can make that fits within the newer class structure of @clarkpede to make this more clear? Comments are great, but they become stale over time, and folks may not read it. It would be good to force folks to consider this somehow while writing the routines. Is there a clever way to error check this for instance?

oleburghardt · 2019-09-25T12:42:35Z

Common/include/option_structure.hpp

 const int HEAT_SOL = 5;		/*!< \brief Position of the heat equation in the solution solver array. */
 const int ADJHEAT_SOL = 6;  /*!< \brief Position of the adjoint heat equation in the solution solver array. */

+const int SCALAR_SOL = 5;    /*!< \brief Position of the scalar transport solution in the solver container array. */


That excludes cases where we might want to run heat and scalar solvers side by side. So is the idea then to have a scalar solver with a (additional) temperature variable or should the weak coupling option be removed?

I think that, in the end, it would be great to make the current heat solver a solver for heat conduction in solids alone by removing the weakly coupled option. This will simplify things throughout the code a bit and let us remove some things. After/with this PR, it should be easier to have the heat solver inherit from some of the base scalar classes, as I have done for the turbulence models.

…e and new output classes. Fixed conflicts.

…it from the CScalar bases.

stale · 2020-01-14T05:31:29Z

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.

economon · 2020-01-15T16:08:06Z

WIP

stale · 2020-03-15T16:18:33Z

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.

economon · 2020-03-17T01:47:01Z

WIP.. some folks in the community are using this as a base for their development

stale · 2020-05-16T02:33:45Z

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.

economon · 2020-05-16T03:41:54Z

Same status as above

stale · 2020-07-15T04:10:40Z

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.

economon added 14 commits August 7, 2018 19:55

Added scalar file stubs and updated build.

be9bd68

CVariable class for scalar transport equations.

1da355e

Scalar transport numerics.

f190118

First implementation of scalar transport solver.

204f844

Added calls to scalar models in iteration_structure.cpp.

924f0ed

Added integration class changes for scalar transport.

c32b4f8

First working version of passive scalar implementation and simple com…

f3c083b

…bustion model. Needs testing.

New options, MG fix for inlets, etc.

069d066

Merge branch 'develop' into feature_scalar

4102c2f

Updating with develop and fixing a couple of memory issues.

1dbd211

Updating the scalar implementation with the develop branch.

b490794

Clean up of scalar framework for PR.

6805674

More clean up.

9129a23

Small changes.

8b1fe53

pr-triage bot added the PR: unreviewed label Aug 27, 2019

pcarruscag reviewed Aug 28, 2019

View reviewed changes

economon changed the title ~~General Scalar Transport Framework~~ WIP General Scalar Transport Framework Aug 28, 2019

pr-triage bot removed the PR: unreviewed label Aug 28, 2019

WallyMaier added the PR: unreviewed label Aug 30, 2019

pr-triage bot removed the PR: unreviewed label Aug 31, 2019

oleburghardt reviewed Sep 25, 2019

View reviewed changes

This was referenced Oct 4, 2019

Restructuring of CVariable into a Contiguous Data Structure #753

Merged

Hybrid OpenMP-MPI implementation Strategy + Vectorization (SIMD) #789

Closed

economon mentioned this pull request Oct 15, 2019

Various fixes #798

Merged

3 tasks

pcarruscag mentioned this pull request Oct 31, 2019

Various fixes II #807

Merged

4 tasks

talbring added the changelog:none label Nov 9, 2019

economon changed the title ~~WIP General Scalar Transport Framework~~ WIP General Framework for Scalar Transport Equations Nov 12, 2019

economon added the changelog:feature label Nov 12, 2019

economon added 2 commits November 13, 2019 00:36

Updated the scalar framework to the new contiguous CVariable structur…

ef7730a

…e and new output classes. Fixed conflicts.

Adjusting classes so that all turbulent CVariable and CNumerics inher…

7741b76

…it from the CScalar bases.

economon added 2 commits November 14, 2019 18:02

Fixed turbulent variables names to be the scalar names for viscous flux.

048713e

Updating TurbVar -> ScalarVar across turb solver BCs.

dde6063

stale bot added the stale label Jan 14, 2020

stale bot removed the stale label Jan 15, 2020

stale bot added the stale label Mar 15, 2020

stale bot removed the stale label Mar 17, 2020

stale bot added the stale label May 16, 2020

stale bot removed the stale label May 16, 2020

stale bot added the stale label Jul 15, 2020

stale bot closed this Jul 22, 2020

economon mentioned this pull request Dec 28, 2020

Info on current status of LM transition model in SU2 #1130

Closed

TobiKattmann mentioned this pull request Jul 21, 2021

Base class for transported scalars #1330

Merged

5 tasks


		/--- Mean flow primitive variables using gradient reconstruction and limiters ---/

		Gradient_i = solver_container[FLOW_SOL]->node[iPoint]->GetGradient_Primitive();

WIP General Framework for Scalar Transport Equations #777

WIP General Framework for Scalar Transport Equations #777

Uh oh!

Conversation

economon commented Aug 27, 2019

Proposed Changes

Related Work

PR Checklist

Uh oh!

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

clarkpede Aug 28, 2019 • 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!

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!

oleburghardt left a comment

Choose a reason for hiding this comment

Uh oh!

oleburghardt Sep 25, 2019 • 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!

clarkpede Sep 25, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

oleburghardt Sep 25, 2019 • 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!

stale bot commented Jan 14, 2020

Uh oh!

economon commented Jan 15, 2020

Uh oh!

stale bot commented Mar 15, 2020

Uh oh!

economon commented Mar 17, 2020

clarkpede Aug 28, 2019 •

edited

Loading

oleburghardt Sep 25, 2019 •

edited

Loading

clarkpede Sep 25, 2019 •

edited

Loading

oleburghardt Sep 25, 2019 •

edited

Loading