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.

Sign up for GitHub

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

Jump to bottom

Some fixes / cleanup of CONV_CRITERIA which has no effect and will be deprecated in 7.2.0 #1238

Merged
merged 11 commits into from
Mar 22, 2021
Merged

Some fixes / cleanup of CONV_CRITERIA which has no effect and will be deprecated in 7.2.0 #1238

merged 11 commits into from
Mar 22, 2021

Conversation

TobiKattmann
Copy link
Contributor

@TobiKattmann TobiKattmann commented Mar 18, 2021 •
edited
Loading

Proposed Changes

  • remove unused CONV_CRITERIA from code and Testcases (223)
  • remove unused Set/GetGeometryPlanes funciton
  • Re-add SURFACE_STATIC_TEMPERATURE objective function for flow_solver

PR Checklist

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

@pr-triage pr-triage bot added the PR: draft label Mar 18, 2021
pcarruscag
pcarruscag reviewed Mar 18, 2021
View reviewed changes
Common/src/CConfig.cpp Outdated
Comment on lines 2887 to 2888
if (!option_name.compare("CONV_CRITERIA"))
newString.append("CONV_CRITERIA is deprecated.\n\n");
Copy link
Member

Choose a reason for hiding this comment

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

Dammmm really?! I guess if something is a residual it uses the residual criteria (criterion?) and if it is a coefficient it uses Cauchy?
Mind throwing that in the message? This is another one that will break every config...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. In COutput::Convergence_Monitoring there is a distinction for CONV_FIELD between HistoryFieldType::COEFFICIENT and HistoryFieldType::RESIDUAL.

I'll add that information to the error msg 👍

Copy link
Member

Choose a reason for hiding this comment

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

actually... do you think we should give folks a pass and just print a warning (with that exact message) and say that we will deprecate in the future? When we go to 7.2.0 (which could happen soon if we merge the hybrid parallel AD and the Scalar solvers)

Copy link
Contributor Author

@TobiKattmann TobiKattmann Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, that also requires readding the option. I'll try to remember to enforce that for 7.2.0
edit: ah and the optionMap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I made it a warning but as it is thrown at config parsing in Postprocessing I guess not lot of people will see it anyway.

I guess one can git revert aa7393f at some point in the future.

And CONV_CRITERIA is a stringOption so one can type in there whatever one likes :) no optionMap required

@TobiKattmann TobiKattmann mentioned this pull request Mar 19, 2021
Merged
@TobiKattmann TobiKattmann marked this pull request as ready for review March 19, 2021 13:14
@pr-triage pr-triage bot removed the PR: draft label Mar 19, 2021
@TobiKattmann TobiKattmann changed the title [WIP] Some fixes / cleanup Some fixes / cleanup Mar 19, 2021
TobiKattmann
TobiKattmann commented Mar 19, 2021
View reviewed changes
SU2_CFD/src/drivers/CMultizoneDriver.cpp
@@ -173,7 +173,7 @@ void CMultizoneDriver::StartSolver() {
if (rank == MASTER_NODE){
cout << endl <<"Simulation Run using the Multizone Driver" << endl;
if (driver_config->GetTime_Domain())
cout << "The simulation will run for " << driver_config->GetnTime_Iter() << " time steps." << endl;
cout << "The simulation will run until time step " << driver_config->GetnTime_Iter() << "." << endl;
Copy link
Contributor Author

@TobiKattmann TobiKattmann Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

In SingleZoneDriver nTimeIter - Restart_Iter can lead to underflow of the unsigned long result which is now checked in CConfig

@pcarruscag pcarruscag changed the title Some fixes / cleanup Some fixes / cleanup of CONV_CRITERIA which has no effect and will be deprecated in 7.2.0 Mar 22, 2021
@TobiKattmann TobiKattmann merged commit 19826aa into develop Mar 22, 2021
@TobiKattmann TobiKattmann deleted the happy_little_fixes branch March 22, 2021 19:46
@TobiKattmann
Copy link
Contributor Author

Thanks for the approval! 🦃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pcarruscag pcarruscag pcarruscag approved these changes

Assignees
No one assigned
Labels
changelog:chore changelog:fix PR: merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TobiKattmann @pcarruscag