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

Fixing Warnings #440

Merged
merged 7 commits into from Sep 15, 2017
Merged

Fixing Warnings #440

merged 7 commits into from Sep 15, 2017

Conversation

ghost
Copy link

@ghost ghost commented Sep 13, 2017

My compiler is detection tens of warnings, I have fixed most of them.

Best,
Vivaan

@ghost ghost changed the title Fixing Warning Fixing Warnings Sep 13, 2017
Copy link
Member

@economon economon left a comment

Choose a reason for hiding this comment

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

Looks good to me.. just one comment inside the review.

Thanks for cleaning up the warnings for the unused variables and initializations!

else
eps2 = eps1*eps1*eps1;
if (config->GetKind_SlopeLimit_Flow() == VENKATAKRISHNAN_WANG) {
eps1 = 0.03 * (GlobalMaxPrimitive[iVar] - GlobalMinPrimitive[iVar]);
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 the VEKATAKRISHNAN_WANG limiter coefficient is hard-coded. Any reason to make this variable, or does the tuning within this new limiter take care of any required scaling?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks!

There is a good reason for that... my frustration using the original Venkat's limiter. The freedom to choose parameters is a double-edged sword... you have control but you can affect the results. With the original method (2 parameters)... I was able to obtain any result!

For that reason, in this new implementation I decided to hardcode the recommended value for the parameter. So... users will have consisten solutions.

By the way, I have some results that point out to the fact that this version is much better than the original Venkat's limiter (convergence and accuracy)... maybe in the near future we should replace the original one by this one.

Best,
Vivaan

Copy link
Contributor

Choose a reason for hiding this comment

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

Dear all,

Thanks for the implementation @VivaanKhatri

However, the eps value in VEKATAKRISHNAN_WANG can be view as percentage of the maximum range of the flow field, oscillations exceeding this value are limited. In my opinion, this value should not be hard-coded and we can use the limiter coefficient from the config file. According to Wang's original "A fast nested multi-grid viscous flow solver", the value of 0.05 is a good starting candidate.

Best,

Eduardo

Copy link
Author

Choose a reason for hiding this comment

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

I have added the option LIMITER_COEFF to the VENKATAKRISHNAN_WANG limiter.

Best,
Vivaan

Copy link
Member

@economon economon left a comment

Choose a reason for hiding this comment

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

Looks good. We can merge this asap.

Just one small change requested first thought with the console output.

@@ -4458,7 +4458,7 @@ void CConfig::SetOutput(unsigned short val_software, unsigned short val_izone) {
cout << "The reference element size is: " << RefElemLength <<". "<< endl;
break;
case VENKATAKRISHNAN_WANG:
cout << "Venkatakrishnan-Wang slope-limiting method, with constant: " << LimiterCoeff <<". "<< endl;
cout << "Venkatakrishnan-Wang slope-limiting method, with constant: 0.03."<< endl;
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding the coefficient back in.. can you please update these cout statements to print the value of LimiterCoeff again?

Copy link
Author

Choose a reason for hiding this comment

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

Done!

Copy link
Member

@talbring talbring left a comment

Choose a reason for hiding this comment

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

Thanks for fixing these issues.

@economon economon merged commit 97b2277 into su2code:develop Sep 15, 2017
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