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

Improve cauchy convergence to handle quantities that converge to small values #1577

Merged

Conversation

ArneVoss
Copy link
Member

Proposed Changes

Give a brief overview of your contribution here in a few sentences.

I believe it would be helpful if the user can choose between relative or absolute values for cauchy convergence.
Example: For the clean aircraft, the rolling moment coefficient MOMENT_X is close to zero and thus will never reach a relative cauchy convergence ->> dividing tiny numbers is not a good idea. Using absolute cauchy convergence is more robust in this case.

At the same time, I understand that a more complex input / more parameters might confuse users. An automatic switch, as propsed in this pull request, is a good compromise. The code now switches automatically to absolut cauchy convergence if the coefficient is < 0.1 to avoid the problem described above. The config file is unchanged.

See short discussion here: https://www.cfd-online.com/Forums/su2/240148-cauchy-convergence-criteria-cmx-cmy-cmz.html.

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.

  • 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.

for coefficients that are close to zero. Example: For the clean
aircraft, the rolling moment coefficient MOMENT_X is close to zero and
thus will never reach a relative cauchy convergence ->> dividing tiny
numbers is not a good idea. Using absolute cauchy convergence is more
robust in this case.
See also short discussion on the CFD-online forum:
https://www.cfd-online.com/Forums/su2/240148-cauchy-convergence-criteria-cmx-cmy-cmz.html
by multiple SU2 runs, leading to io conflicts. Thus, the name of the
temporary file should not be hard-coded. This bugfix simply adds a
'_tmp' to the original config file name.
@ArneVoss ArneVoss changed the base branch from master to develop March 23, 2022 15:13
SU2_CFD/src/output/COutput.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/output/COutput.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/output/COutput.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/output/COutput.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/output/COutput.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/output/COutput.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/output/COutput.cpp Outdated Show resolved Hide resolved
Copy link
Member Author

@ArneVoss ArneVoss left a comment

Choose a reason for hiding this comment

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

Thank you for your review and for shortening/simplifying the code! Do I need to agree to your propsal? How do I do this?

@pcarruscag pcarruscag changed the title Feature improved cauchy convergence Improve cauchy convergence to handle quantities that converge to small values Mar 24, 2022
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.

No need to aprove, it is already applied.
As with many similar constants, having the limit at 0.1 is a bit arbitrary, but it is better than diving by zero for sure.
Because this may change the way the code behaves (some folks may have tuned the convergence criteria for some small functions) let's wait for a bigger release (7.4.0) to merge this PR, it is very local so there should not be any issues.
We will probably have 7.3.1 this month (with mostly just bug fixes), and we should have 7.4.0 not long after.

Thank you again for contributing to SU2. By the way, we have weekly developer meetings, Wednesdays at 4pm CET, you are welcome to join if you want (https://meet.jit.si/SU2_DevMeeting)

@ArneVoss
Copy link
Member Author

OK, great, thank you!

@pcarruscag pcarruscag merged commit 2ce680a into su2code:develop May 15, 2022
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.

2 participants