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.

Sign up for GitHub

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

Jump to bottom

Fix dissipation in transition model and update inlet profile (initial profile from config) #1268

Merged
merged 13 commits into from
Apr 21, 2021
Merged

Fix dissipation in transition model and update inlet profile (initial profile from config) #1268

merged 13 commits into from
Apr 21, 2021

Conversation

bigfooted
Copy link
Contributor

Proposed Changes

  1. fixed factor of 100 in the transition turbulence models
  2. changed initialization of inlet profile from 0.0 to values taken from config file. I guess this was done to make this routine more general (for other boundaries) in the future. However, I think generating a default file that works out of the box is better.
  3. added a descriptive comment line to the inlet profile file (one for each marker) with the names of the columns. This line is intended to make editing easier for the user. This comment line is not mandatory and is ignored by the profile reader.
  4. do not write residuals of temperature to output file when energy equation is off

Related Work

Related to #1263 and #1265

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.

pcarruscag
pcarruscag requested changes Apr 18, 2021
View reviewed changes
SU2_CFD/include/CMarkerProfileReaderFVM.hpp Outdated Show resolved Hide resolved
SU2_CFD/src/CMarkerProfileReaderFVM.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/numerics/turbulent/turb_sources.cpp
@@ -139,7 +139,8 @@ CNumerics::ResidualType<> CSourcePieceWise_TurbSA::ComputeResidual(const CConfig
const su2double chi_1 = 0.002;
const su2double chi_2 = 50.0;

su2double tu = config->GetTurbulenceIntensity_FreeStream();
/*--- turbulence intensity is u'/U so we multiply by 100 to get percentage ---*/
su2double tu = 100.0 * config->GetTurbulenceIntensity_FreeStream();
Copy link
Member

Choose a reason for hiding this comment

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

👍 every now and then CFD-online helps us find some bugs :)

pcarruscag
pcarruscag approved these changes Apr 20, 2021
View reviewed changes
SU2_CFD/src/solvers/CSolver.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CSolver.cpp Outdated
Comment on lines 3363 to 3364
INLET_TYPE Kind_Inlet = config->GetKind_Inlet();
switch (Kind_Inlet) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
INLET_TYPE Kind_Inlet = config->GetKind_Inlet();
switch (Kind_Inlet) {
switch (config->GetKind_Inlet()) {

SU2_CFD/src/solvers/CSolver.cpp
Comment on lines +3359 to +3360
columnName << "# COORD-X " << setw(24) << "COORD-Y " << setw(24);
if(nDim==3) columnName << "COORD-Z " << setw(24);
Copy link
Member

Choose a reason for hiding this comment

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

This is going to sound like neat picking in this particular context but bear with me (you never know the next piece of code you will work on).
From an efficiency standpoint you should always move as little data as possible (talking about the spaces in setw and passing numbers as strings).
This is particularly relevant for strings because of something called "small string optimization", where up to a certain number of characters, strings do not allocate memory on the heap.
In this example, this also means that you are forcing the file format from the outside, this should be the responsibility of the class (profile reader in this case) consider for example that we may want to use it for CSV formats.

SU2_CFD/src/solvers/CSolver.cpp Outdated
columnName << "# COORD-X " << setw(24) << "COORD-Y " << setw(24);
if(nDim==3) columnName << "COORD-Z " << setw(24);

if (compressible){
Copy link
Member

Choose a reason for hiding this comment

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

This is ok for now, but if more solvers start using inlet profiles this is not a scalable solution.
The suggestion was to have this block of code as a method of different solvers, so that you do not have to figure out the type of solver based on configuration options (and consequently the type of inlet, etc. etc.).
solver->GetInletVariableNames() something like that.
The motivation for polymorphism and abstract classes is to allow centralizing this type of knowledge, i.e. "what type of object is this?", on a single place, the instantiation of the object.
Enough philosophy...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I prefer having something like this at the level of the solver container so we can construct this information. After all, very soon we will have passive scalar, combustion variables, nemo combustion variables,...

@pcarruscag
Copy link
Member

By the way please give the PR a more descriptive title that reflects the transition fix and the column names feature.
So that folks know what to expect from the change log.

@bigfooted @pcarruscag
Update SU2_CFD/src/solvers/CSolver.cpp
0f3c962 
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
@bigfooted bigfooted changed the title Fix turbulence input Fix dissipation in transition model and update inlet profile (initial profile from config) Apr 21, 2021
@bigfooted bigfooted merged commit 1c3610d into develop Apr 21, 2021
@bigfooted bigfooted deleted the fix_turbulence_input branch April 21, 2021 11:59
This was referenced Apr 21, 2021
Closed
Closed
TobiKattmann
TobiKattmann reviewed Apr 28, 2021
View reviewed changes
Copy link
Contributor

@TobiKattmann TobiKattmann left a comment

Choose a reason for hiding this comment

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

A bit late, but thanks for the transition-model bugfix and the inlet-marker-stuff 💐

SU2_CFD/src/CMarkerProfileReaderFVM.cpp
Comment on lines 47 to +51
filename = val_filename;
markerType = val_kind_marker;
numberOfVars = val_number_vars;
columnNames = val_columnNames;
columnValues = val_columnValues;
Copy link
Contributor

@TobiKattmann TobiKattmann Apr 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I guess this could be done with an initializer list for the ctor .. but I am unsure if one is to be preferred over the other. The core guidelines dont seem to have a strong opinion either. CE Playground

SU2_CFD/src/CMarkerProfileReaderFVM.cpp
Comment on lines 130 to 134
}
} else {
SU2_MPI::Error("While opening profile file, no \"NMARK=\" specification was found", CURRENT_FUNCTION);
//cout << "inlet profile reader is ignoring line: " << text_line << endl;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

in that case I would remove the whole else statement ... unless there are further plans

SU2_CFD/src/output/CFlowIncOutput.cpp
Comment on lines 596 to +601
SetVolumeOutputValue("RES_VELOCITY-Z", iPoint, solver[FLOW_SOL]->LinSysRes(iPoint, 3));
SetVolumeOutputValue("RES_TEMPERATURE", iPoint, solver[FLOW_SOL]->LinSysRes(iPoint, 4));
if (config->GetEnergy_Equation())
SetVolumeOutputValue("RES_TEMPERATURE", iPoint, solver[FLOW_SOL]->LinSysRes(iPoint, 4));
} else {
SetVolumeOutputValue("RES_TEMPERATURE", iPoint, solver[FLOW_SOL]->LinSysRes(iPoint, 3));
if (config->GetEnergy_Equation())
SetVolumeOutputValue("RES_TEMPERATURE", iPoint, solver[FLOW_SOL]->LinSysRes(iPoint, 3));
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess one could move this config->GetEnergy_Equation() out of the if nDim and make the acces solver[FLOW_SOL]->LinSysRes(iPoint, nDim+1)

TobiKattmann added a commit that referenced this pull request Apr 28, 2021
@TobiKattmann
Two remarks to PR #1268 of @bigfooted
f945eb1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TobiKattmann TobiKattmann TobiKattmann left review comments

@pcarruscag pcarruscag pcarruscag approved these changes

Assignees
No one assigned
Labels
changelog:feature PR: unreviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bigfooted @pcarruscag @TobiKattmann