Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Base class for transported scalars #1330

Merged
merged 64 commits into from
Sep 22, 2021
Merged

Base class for transported scalars #1330

merged 64 commits into from
Sep 22, 2021

Conversation

TobiKattmann
Copy link
Contributor

@TobiKattmann TobiKattmann commented Jul 21, 2021

Proposed Changes

Hi all,

The general idea for this PR can be cited from @pcarruscag in #1226:

The ideia would be to create a CFVMSolverBase (in the style of the one for flow solvers) to cover:

    Turbulence solvers;
    Heat solver;
    Radiation solver;
    General passive/active scalar solvers;
    Eventual transition solvers;

The starting point would be the current base turbulence solver.

Right now the only things I did were (i.e. don't fear the large diff)

  • create CScalarSolver/Variable files/classes and scalar_convection/diffusion/sources files and fill them with the contents of the CTurb* or turb_* files.
  • Make CScalarSolver/Variable the parent class of CTurbSolver/Variable
  • Delete pretty much everything from CTurb* or turb_* files except for some constructors

So far it compiled and some local regression testing showed no problems, lets see what the CI pipeline has to say about this

Now with this additional class introduced the actual works begins:

  • figure out what is a reasonable interface in CScalar* to unify transported species and turb-models (and the other solvers probably)
  • code this common interface and push all turb-specific things back into the respective files (that is why I didn't git mv )

In order to keep this somewhat limited I would focus on porting the Turbsolvers to the new structure (however that might look, but looking at #1223 is what I'll do first) and maybe a simple transported passive scalar or so. We want the turb solver to be integrated under that Scalar-class which is arguably what most/all people use, that's why I want to have in this work at the beginning. We also have quite a few RANS Testcases so it is directly visible if something in the actual computation is changed.

In case there are comments, ideas foreseeable stepping stones from experience please let me know here.

Related Work

#777 by @economon which is the initial push towards this separate scalar solver class for additionally transported scalars (not merged)
#1223 by @danielmayer @bigfooted which builds up on #777 and extends it to its needs (but does not support the turb solvers e.g.)
#1226 by @oleburghardt - In this PR is a discussion on creating the structure which this (and following) PR('s) try to achieve
#1332 by @mheimgartner - implementing multi component flow (or analytic mixing properties of said multicomp flow). That PR is built upon #1223 but will merge this PR (once it is done) and throw out all additional changes in order to be merged into develop without relying on feature_flamelet

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.

@TobiKattmann TobiKattmann marked this pull request as draft July 21, 2021 15:55
@TobiKattmann TobiKattmann changed the title Base class for transported scalars [WIP] Base class for transported scalars Jul 21, 2021
@TobiKattmann TobiKattmann changed the title [WIP] Base class for transported scalars Base class for transported scalars Aug 12, 2021
@TobiKattmann
Copy link
Contributor Author

Still WIP but with that in the title the regression status behind each commit is always "unfinished" because WIP-check-bot does that. Now that we have Draft-PR I guess we dont WIP anymore tbh

@pcarruscag
Copy link
Member

It helps me with the manual edit of the release notes, to know what was not merged yet.

@pcarruscag pcarruscag changed the title Base class for transported scalars [WIP] Base class for transported scalars Aug 12, 2021
Allowing optimized acces to muT of TurbSolvers.
Implementation follows what is done for CFVMFlowSolverBase.
And move current Turb specific impl to CTurbSolver. I.e. each Child of
CscalarSolver has to implement its own LoadRestart. One has to make sure
the correct Pre/Postprocessign routines are called in there which are
consistent with the repective C(Fluid)Iteration.
SU2_CFD/include/solvers/CScalarSolver.hpp Outdated Show resolved Hide resolved
SU2_CFD/include/solvers/CScalarSolver.hpp Outdated Show resolved Hide resolved
SU2_CFD/include/solvers/CScalarSolver.hpp Outdated Show resolved Hide resolved
SU2_CFD/include/solvers/CScalarSolver.inl Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CTransLMSolver.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CTurbSASolver.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CTurbSolver.cpp Show resolved Hide resolved
SU2_CFD/src/solvers/CTurbSolver.cpp Show resolved Hide resolved
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CTurbSSTSolver.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CTurbSASolver.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CTransLMSolver.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CTransLMSolver.cpp Outdated Show resolved Hide resolved
…ates


Code suggestions by pcarruscag

Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
TobiKattmann and others added 4 commits September 22, 2021 15:25
To stay consistent with the .hpp. One could also use a more concise <V>
as suggested by pcaruscag but I prefer to stay use the same name here.
Copy link
Member

@pcarruscag pcarruscag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work 👍 can't wait to see the crash diet that some solvers are about to go on.

@TobiKattmann
Copy link
Contributor Author

TobiKattmann commented Sep 22, 2021

Thanks @pcarruscag for making this PR a lot better.

@bigfooted @danielmayer @mheimgartner
To layout future steps as discussed after this merge:

  1. Merge these changes into feature_flamelet (and then feature_multicomp). Therefore the CScalar* things that are currently present in these branches will be renamed to CLegacyScalar* to not overwrite files. This has the advantage that those branches are kept up-to-date with develop.
  2. Implement a rudimentary (passive) CSpeciesSolver (specifically does not contain multicomponent mixing from [WIP] Composition dependent fluid properties for multicomponent flow  #1332 ) and merge that. Finding a good (validation) Testcase would be great here.
  3. Extend the CSpeciesSolver by [WIP] Composition dependent fluid properties for multicomponent flow  #1332 mixing models. Either by cutting away all the flamelet specific parts or by porting the additions to a new PR.
  4. Make feature_flamelet switch to the new structure. This is an in-place change as both implementations are then present but just one will be kept. This will shrink the PR size of [WIP]: Flamelet progress variable (FPV) model #1223 by quite a bit

Additionally the heat and radiation solver should be ported into this new structure, but that is independent work.

@TobiKattmann TobiKattmann merged commit f361b13 into develop Sep 22, 2021
@TobiKattmann TobiKattmann deleted the feature_scalarbase branch September 22, 2021 16:07
TobiKattmann added a commit that referenced this pull request Sep 23, 2021
PR 1330 introduces a general scalar framework that has files with the
same name (i.e. CScalarSolver) but that have slightely different scopes.
Therefore a renaming of certain current files with the *Legacy addition
is done to avoid merge conflicts. Merging #1330 which is already in
develop means that we keep up with develop which is wanted.
This means we will introduce the 'new' species/scalar solver into
feature_flamelet with the old one still present.
TobiKattmann added a commit that referenced this pull request Sep 23, 2021
The new one is AddClippedSolutionLegacy. I want to keep the two
implementation fully seperate for now in order to not introduce some
unnecessary bugs as there are no regression tests.
TobiKattmann added a commit that referenced this pull request Sep 23, 2021
This is again to separate the new(#1330) and old (feature_flamelet)
scalar structure. This is just a temporary thing to have a
better/cleaner merge. Afterwards it is certainly desirable to remove the
Legacy varaibles alltogether.
TobiKattmann added a commit that referenced this pull request Sep 23, 2021
Additionally, Remove AddClippedSolutionLegacy with newer version. And
get rid of ScalarVarLegacy(_Grad)_i,j. All of those were introduced
directly prior to the merge in order to make the merge itself have less
conflicts. Both, #1330 the new scalar framework and this
feature_flamelet impl had the same vars/methods which were
similar/identical in use.
@maxaehle maxaehle mentioned this pull request Nov 11, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants