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

Branch rans-dev PR, incomplete #45

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open

Branch rans-dev PR, incomplete #45

wants to merge 33 commits into from

Conversation

garobed1
Copy link
Collaborator

@garobed1 garobed1 commented Feb 4, 2021

This is a pull request to merge rans-dev into dev as it is. I merged the latest dev branch into rans-dev prior to this, and all tests pass.

I leave the branch in an essentially incomplete state since I am going to work with another code for the sake of my upcoming MDO conference paper. While the Spalart-Allmaras turbulence model as it is implemented here produces correct results, the solver strategy is not robust and very often fails when trying to solve problems on RANS-type meshes. I believe the best way to improve robustness is to implement a feature that decouples the Navier-Stokes equations and the SA equation for the start of the solution and solves two separate systems, then recouples the equations after a sufficient residual norm decrease and finishes solving. I may revisit this myself after the paper.

All of the RANS tests that are active pass, although I disabled a few of the tests in test_rans_integ.cpp: SAViscousSlipWallBC Jacobian and SASourceIntegrator::AssembleElementGrad. I am uncertain why the slip wall integrator test fails, but I am reasonably certain that the source term integrator fails due to a finite-differencing error, but I haven't been able to directly prove this.

I haven't added a regression test for the SA model due to the volatility of the solver. I can try to make one if it would be helpful, along with making other requested changes before merging.

Garo Bedonian and others added 30 commits July 8, 2020 13:07
…on confirmed not to break with the extra variable
… problem doesn't converge even with just the EulerSolver
… terms. wall test problems show mesh dependence, and lack of convergence with small wall elements as a result
Conflicts:
	test/unit/euler_test_data.cpp
	test/unit/euler_test_data.hpp
Conflicts:
	src/physics/fluidflow/euler.cpp
@jehicken
Copy link
Contributor

jehicken commented Feb 4, 2021

@garobed1 and @tuckerbabcock Can you take a look at the details of the build failure? It looks to be related to Adept configuration. Thanks.

Copy link
Member

@tuckerbabcock tuckerbabcock left a comment

Choose a reason for hiding this comment

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

I just skimmed this and left a few comments. I'll probably address the two requested changes for the documentation just so the CI gets re-triggered to see if it will pass now.

{"mu", -1.0}, // nondimensional viscosity (if negative, use Sutherland's)
{"sa-consts", {0.1355, 0.622, 0.666666666666667, 0.41, 0.3, 2, 7.1, 1.2, 0.5, 10, 16, 0.7, 0.9}},
//Spalart-Allmaras turbulence model constants, defined in rans_fluxes.hpp
{"sa-srcs", {1.0, 1.0}} //for debug purposes, disable SA source terms
Copy link
Member

Choose a reason for hiding this comment

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

I don't personally like this a default option, I think it can just be an undocumented option that the SA solver can use

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Which one are you referring to, sa-consts, sa-srcs, or both?

Copy link
Member

Choose a reason for hiding this comment

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

I mean the sa-srcs. If it's just something for debugging then I would rather it not be part of the "public" interface of default options.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's fair. As of now it is 'required' to be set, but I think I'll just remove the option altogether since I know the solver is correct now.

@tuckerbabcock
Copy link
Member

It looks like the previous issue with the CI on this PR (related to Adept configure) has resolved itself, and the new issue is the issue with Hypre not being compiled with fPIC. Once #47 is merged into dev we can merge dev back into rans-dev and the tests should be able to compile.

@tuckerbabcock
Copy link
Member

tuckerbabcock commented Apr 7, 2021

I merged dev into this branch so that the CI would run, and it seems to fail on one of the new RANS related tests, SAViscousSlipWallBC. The numbers look very close, so it could just be a finite difference issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants