SU2 icon indicating copy to clipboard operation
SU2 copied to clipboard

Fix CFL Turbulence reduction option for adaptive cfl

Open emaberman opened this issue 1 year ago • 6 comments

Proposed Changes

Extend the Turbulence CFL reduction option, such that it can be used for adaptive CFL too.

Turbulence cfl reduction was previously implemented only for a constant cfl, here the treatment is extended also for adaptive cfl.

Related Work

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

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.

  • [X] I am submitting my contribution to the develop branch.
  • [X] My contribution generates no new compiler warnings (try with --warnlevel=3 when using meson).
  • [X] My contribution is commented and consistent with SU2 style (https://su2code.github.io/docs_v7/Style-Guide/).
  • [X] I used the pre-commit hook to prevent dirty commits and used pre-commit run --all to format old commits.
  • [] 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.

emaberman avatar Jun 05 '24 09:06 emaberman

Thanks for the contribution, add yourself to AUTHORS.md please.

pcarruscag avatar Jun 05 '24 15:06 pcarruscag

Please check and update the regression tests before merging.

bigfooted avatar Jun 09 '24 09:06 bigfooted

I am having trouble with local regression testing. When running regression tests locally on my computer following instructions in the documentation many tests fail, including ones that seem to pass when attempting to upload. I seem to be missing something.. What am I doing wrong?

emaberman avatar Jun 20 '24 09:06 emaberman

The test values are sensitive to compiler versions and options. Just run the failing cases locally and if the results looks reasonable update the results based on the github tests.

pcarruscag avatar Jun 20 '24 14:06 pcarruscag

There is one case that has different residuals, the Jones turbocharger case. The other case is a know problem that is fixed in another PR. So you should check if this case uses adaptive CFL and CFL_REDUCTION_TURB, because that is when the residuals could be different. Please also check if it still converges, because it looks like it 's not. So maybe it needs another CFL number?

bigfooted avatar Jun 20 '24 17:06 bigfooted

The jones_turbocharger_restart case was set to use adaptive CFL coupled with CFL turbulence Reduction thus changes in the results are expected , I ran the case a few thousand steps and the case seems to converge. Therefore I am allowing myself to change test results leaving the configuration setting as is.

emaberman avatar Jun 24 '24 08:06 emaberman

We are wondering what the status with the merge request is. Am I supposed to do something more? is it just waiting for an additional review?

Additionally, we have many more proposed modifications, we have worked on, adding robustness to the existing turbulence models. It is expected, many of these changes will effect the test cases behaviour, are there any guidelines how to compile the code to run the full set of tests locally, or should I just open pull requests, and base upon the github test results?

I am sorry if I may be asking the obvious, but I am new to contributing to open code..

emaberman avatar Jul 10 '24 05:07 emaberman

The PR is approved. Do you see a button saying "Merge pull request"? You can use docker containers to run tests locally in a reproducible way, but I always used github to run all the tests for me, so yes open draft PRs as you work on more changes. You forgot to add yourself to AUTHORS.md. If you plan to contribute more I'll add you to the SU2 organization and then you can push branches to su2code instead of using your fork.

pcarruscag avatar Jul 10 '24 14:07 pcarruscag

Thank you Yes we plan on contributing more, this commit was more of a test run just to understand the process. I added Dr Yair Mor Yossef and myself as Authors, per your request. However I do not see a button anywhere that says merge pull request (even before changing Authors.md)

emaberman avatar Jul 10 '24 16:07 emaberman

Thank you Yes we plan on contributing more, this commit was more of a test run just to understand the process. I added Dr Yair Mor Yossef and myself as Authors, per your request. However I do not see a button anywhere that says merge pull request (even before changing Authors.md)

It should be below this message,You should see the list of 'things that need to be green': Changes Approved, Conversations resolved, and then Checks passed. At the moment I am writing this, the checks are still running. Below the Checks you should see the button "Squash and Merge". Once all checks have passed, you can press that button.

bigfooted avatar Jul 10 '24 18:07 bigfooted

I"m not sure what I"m missing here, All the tests have past but no such button seems to exist anywhere..
Could this be due to the fact that I'm an Author and not a contributor?

emaberman avatar Jul 11 '24 07:07 emaberman