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

Streamwise periodicity for incompressible flow #773

Merged
merged 297 commits into from
Mar 2, 2021

Conversation

TobiKattmann
Copy link
Contributor

@TobiKattmann TobiKattmann commented Aug 26, 2019

Hi,

this PR adds streamwise periodic flow (incl energy eq) for the incompressible solver. For a more detailed information please see the website additions including a tutorial and theory section.

~half of the lines is .cfg's as those CHT setups require a bit more real estate. So the actual code is not too long.

Here a squash merge could be done as it merged develop quite a few times and has a bunch of 'small fix' commits. But to my knowledge that is not really necessary as.

I am ready to pass on the red lantern to someone else 🐌

Related Work

This work builds directly on #652 of @economon

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).
  • My contribution is commented and consistent with SU2 style.
  • I have added a test case that demonstrates my contribution, if necessary.

@pr-triage pr-triage bot added the PR: draft label Aug 26, 2019
…treamwise

Conflicts:
	SU2_CFD/include/solver_structure.hpp
	SU2_CFD/include/variables/CVariable.hpp
	SU2_CFD/src/solver_direct_mean_inc.cpp
…treamwise

Conflicts:
	SU2_CFD/src/output_structure.cpp
…treamwise

Conflicts:
	SU2_CFD/src/numerics_direct_mean_inc.cpp
…treamwise

Conflicts:
	Common/include/config_structure.hpp
…treamwise

Conflicts:
	.travis.yml
	SU2_CFD/include/variables/CIncEulerVariable.hpp
	SU2_CFD/src/output/output_structure_legacy.cpp
	SU2_CFD/src/solver_direct_mean_inc.cpp
…treamwise

Conflicts:
	Common/src/config_structure.cpp
	SU2_CFD/include/variables/CEulerVariable.hpp
	SU2_CFD/src/output/output_structure_legacy.cpp
	SU2_CFD/src/solver_direct_mean_inc.cpp
	SU2_CFD/src/variables/CIncEulerVariable.cpp
	TestCases/serial_regression.py
…iodic_streamwise

Conflicts:
	SU2_CFD/src/output/CFlowIncOutput.cpp
	SU2_CFD/src/solver_adjoint_discrete.cpp
	SU2_CFD/src/solver_structure.cpp
@TobiKattmann TobiKattmann changed the title [WIP] Streamwise periodicity for incompressible flow Streamwise periodicity for incompressible flow Feb 26, 2021
@TobiKattmann
Copy link
Contributor Author

I guess one can ignore the CodeFactor complaints here as it just gives a vague complex code without any further line information error for CIncEulerSolver.cpp (which I could understand to some degree) and CSolver.hpp (which I dont understand at all).

Common/src/geometry/CPhysicalGeometry.cpp Outdated Show resolved Hide resolved
Common/src/geometry/CPhysicalGeometry.cpp Outdated Show resolved Hide resolved
Common/src/geometry/CPhysicalGeometry.cpp Outdated Show resolved Hide resolved
Common/src/geometry/CPhysicalGeometry.cpp Outdated Show resolved Hide resolved
Common/src/geometry/CPhysicalGeometry.cpp Outdated Show resolved Hide resolved
SU2_CFD/include/variables/CVariable.hpp Show resolved Hide resolved
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.

Alright I think I covered everything.
World class documented code 👍
Loads of comments on this second pass, but they are just small re-arrangements to encapsulate the feature a bit better in terms of code, and in terms of side effects when not using it.

Common/include/CConfig.hpp Outdated Show resolved Hide resolved
Common/src/geometry/CPhysicalGeometry.cpp Outdated Show resolved Hide resolved
SU2_CFD/include/solvers/CSolver.hpp Outdated Show resolved Hide resolved
SU2_CFD/src/output/COutput.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CIncEulerSolver.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CIncNSSolver.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CIncNSSolver.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CIncNSSolver.cpp Outdated Show resolved Hide resolved
Comment on lines -482 to +485
residual[nDim] = Volume * U_i[0] * STANDARD_GRAVITY;
residual[nDim] = Volume * U_i[0] * STANDARD_GRAVITY / Force_Ref;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a bug fix 👍

Common/include/CConfig.hpp Outdated Show resolved Hide resolved
@pcarruscag
Copy link
Member

No need to squash, the testcases repo is actually a bit messed up ATM because of that (oooops)

@@ -716,6 +716,7 @@ class CConfig {
unsigned short Geo_Description; /*!< \brief Description of the geometry. */
unsigned short Mesh_FileFormat; /*!< \brief Mesh input format. */
unsigned short Tab_FileFormat; /*!< \brief Format of the output files. */
unsigned short output_precision; /*!< \brief <ofstream>.precision(value) for SU2_DOT and HISTORY output */
Copy link
Contributor Author

@TobiKattmann TobiKattmann Feb 26, 2021

Choose a reason for hiding this comment

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

There is possibly other locations (edit: beyond history and SU2_DOT) where one can plug this, but it is a start for gradient validation

/*-------------------------------------------------------------------------------------------*/

/*--- Initialize/Allocate variables. ---*/
su2double min_norm = numeric_limits<su2double>::max();
Copy link
Member

Choose a reason for hiding this comment

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

Ah! no way, they've defined numeric_limits for the AD types, those guys rock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

\o/

SU2_CFD/include/solvers/CIncEulerSolver.hpp Outdated Show resolved Hide resolved
Comment on lines 93 to 94
/*--- Incompressible flow, gradients primitive variables nDim+4, (P, vx, vy, vz, T, rho, beta) ---*/
/*--- Incompressible flow, gradients primitive variables nDim+6, (P, vx, vy, vz, T, rho, beta, lamMu, EddyMu) ---*/
Copy link
Member

Choose a reason for hiding this comment

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

insert emoji with monocle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🧐 there you go

Copy link
Member

Choose a reason for hiding this comment

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

Where the heck do you get them from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:monocle_face

Copy link
Contributor Author

Choose a reason for hiding this comment

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

most of the time I just put the : and type want I want to have (crazy, right :) ) those github emoji cheatsheets are often incomplete

Copy link
Member

Choose a reason for hiding this comment

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

🤦 here I was just trying to scroll down after typing :

@TobiKattmann
Copy link
Contributor Author

Thanks a lot on the comments so far @pcarruscag . Helps quite a bit 👍
I think I addressed all your comments by now (using AuxVarGrad, a bunch of openMP instructions, overhauling the FindUnique_RefNode func, making a struct for the streamwise periodic values and using that in Numerics and the solver, not using config as a solver var container, and some other things )
Feel encouraged to comment/request-changes on more things.

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.

I would not have written / documented / tested better myself.
I'll aprove it again for good measure.

@pcarruscag
Copy link
Member

All done? Is CodeFactor not letting you click the magic button?

@TobiKattmann TobiKattmann merged commit fa4e6e4 into develop Mar 2, 2021
@TobiKattmann TobiKattmann deleted the feature_periodic_streamwise branch March 2, 2021 15:48
@TobiKattmann
Copy link
Contributor Author

There we go :)
The website stuff is no merged as well, so that can go live with the next master release.
Thanks again.

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