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

Fixes for incompressible solver - rotating frame and convergence rate for unsteady problems #1067

Merged
merged 19 commits into from
Dec 2, 2020

Conversation

cvencro
Copy link
Contributor

@cvencro cvencro commented Aug 16, 2020

Proposed Changes

The draft pull request has changes to fix couple of issues in the incompressible solver for rotating frame and convergence rate of unsteady problems (both with and without grid movement).

For the rotating frame, there was a confusion between conservative and primitive variables being used for the numerics.

For the convergence rate of unsteady problems, the Jacobian contributions in the dual time residual calculations had been altered to include the preconditioner. However, this slows down the convergence from only an order of 100 iterations needed for converged solution per time step to the order of 1000 iterations. Therefore this has been reverted to regain the previous faster convergence rate and further work is necessary to understand the reason for this change.

Related Work

PR #652

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

@pr-triage pr-triage bot added the PR: draft label Aug 16, 2020

numerics->SetConservative(nodes->GetSolution(iPoint), nullptr);
numerics->SetPrimitive(nodes->GetPrimitive(iPoint), nullptr);
Copy link
Member

Choose a reason for hiding this comment

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

nice catch 👍

}
// ADD old version

if (implicit && false) {
Copy link
Member

Choose a reason for hiding this comment

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

For the Jacobian change, let's go with the data: sounds like the version without the preconditioner is an order of magnitude better in your tests, so I would make it the default here and remove the old implementation from the code (remember we can always recover to from the git history).

It is likely that adding the preconditioner portion is hurting the diagonal dominance of the system, or we have a bug / logic error that would be good to understand once there is time to do more analysis

Copy link
Contributor

@TobiKattmann TobiKattmann Aug 18, 2020

Choose a reason for hiding this comment

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

The version without Preconditioner is the "exact" Jacobian dR/dV imo. I recall that (artificially) using the preconditioner was s.th. we / I / somebody tested because there is no Jacobian contribution for the continuity equation (iVar=1).
So we could have an unlucky case where the artificial Jacobian is hurting convergence compared to the exact one?
I looked in the Paper of Smith&Weiss for additional info without much success :)

Copy link
Member

Choose a reason for hiding this comment

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

Gonna stick my nose in :) You guys can also consider an approach similar to the associated with CENTRAL_JACOBIAN_FIX_FACTOR, i.e. multiply this Jacobian contribution with a weight on the off chance some problem might benefit from it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Pedro, I've added an option for the fix factor in centered numerics of the incompressible methods. The preconditioner acts as the multiplication factor already, but there is now an option for an additional fix factor. For a test case I'm currently using, factor below 1 has small improvements to convergence but not a significant step change so the default value for CENTRAL_INC_JACOBIAN_FIX_FACTOR in the config is 1.0. But like you say having this option will give more tweaking flexibility in the config in case it benefits some other problems.

Copy link
Member

Choose a reason for hiding this comment

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

That was not what I meant... maybe I can explain better this afternoon.

@stale
Copy link

stale bot commented Nov 20, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still a relevant issue please comment on it to restart the discussion. Thank you for your contributions.

@stale stale bot added the stale label Nov 20, 2020
@pcarruscag
Copy link
Member

@cvencro @TobiKattmann did you want to include anything else here or can it be "un drafted"?

@cvencro cvencro removed the PR: draft label Dec 1, 2020
@cvencro
Copy link
Contributor Author

cvencro commented Dec 1, 2020

Hi, there were no other changes from my side so I've removed the draft label.

If there are any other suggestions for improving the convergence (I think there is still an open discussion above), we can either act on it here or just make a note of it for future work.

@cvencro cvencro marked this pull request as ready for review December 1, 2020 17:19
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.

LGTM then, having two options with similar names is not ideal but is the default value is different I don't see another way.

@pcarruscag
Copy link
Member

One regression will likely have to be re-updated.

@TobiKattmann
Copy link
Contributor

One regression will likely have to be re-updated.

Yep, I'll update the CHT unsteady case once the checks completed.

@TobiKattmann TobiKattmann merged commit 6a73909 into develop Dec 2, 2020
@TobiKattmann TobiKattmann deleted the fix_inc_rotatingframe branch December 2, 2020 12:27
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.

4 participants