-
Notifications
You must be signed in to change notification settings - Fork 847
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
Enhancements for the fixed CL mode #780
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @jayantmukho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jayantmukho Thanks for the improvements! Though I have some comments, see below.
@@ -2820,10 +2820,11 @@ bool CFlowOutput::WriteVolume_Output(CConfig *config, unsigned long Iter){ | |||
return true; | |||
} | |||
} else { | |||
return ((Iter > 0) && Iter % config->GetVolume_Wrt_Freq() == 0); | |||
if (config->GetFixed_CL_Mode() && config->GetFinite_Difference_Mode()) return false; | |||
return ((Iter > 0) && Iter % config->GetVolume_Wrt_Freq() == 0) || force_writing; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't the cleanest implementation but is the least intrusive as it is mostly contained in CFlowOutput. @talbring
The other way to do this (which I have a slight preference towards) would be to add an option to disable volume output completely in COutput itself. I would prefer it because it would be a more general function that could possibly be used in other situations as well. But it would be a pretty powerful option that might cause conflicts with other options/solvers if not handled with care.
Unfortunately, the way it is now, there is one state variable (Finite_Difference_Mode) that is stored in the config @pcarruscag . But everything else is contained in the solver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am ok with that!
void CFlowCompOutput::SetAdditionalScreenOutput(CConfig *config){ | ||
|
||
if (config->GetFixed_CL_Mode()){ | ||
SetFixedCLScreenOutput(config); | ||
} | ||
} | ||
|
||
void CFlowCompOutput::SetFixedCLScreenOutput(CConfig *config){ | ||
PrintingToolbox::CTablePrinter FixedCLSummary(&cout); | ||
|
||
if (fabs(historyOutput_Map["CL_DRIVER_COMMAND"].value) > 1e-16){ | ||
FixedCLSummary.AddColumn("Fixed CL Mode", 40); | ||
FixedCLSummary.AddColumn("Value", 30); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved all the output to the CFlowCompOutput. I had to make a few new functions but I tried to copy the way the WriteAdditionalFiles function works. I hope this is what you had in mind @talbring
It prints everytime the angle of attack changes and looks like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep looks good
@@ -7320,71 +7310,51 @@ void CEulerSolver::SetActDisk_BCThrust(CGeometry *geometry, CSolver **solver_con | |||
void CEulerSolver::SetFarfield_AoA(CGeometry *geometry, CSolver **solver_container, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function got cleaned up, and now it does only SetFarfieldAoA (As opposed to perform all the logic for the Fixed CL mode). Moved all that to the FixedCL_Convergence function. Which now that I say out loud, should be name FixedCL_Driver or something of that sort
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jayantmukho. All of that looks good to me.
.travis.yml
Outdated
@@ -72,11 +72,11 @@ install: | |||
|
|||
before_script: | |||
# Get the test cases | |||
- git clone --depth=1 -b develop https://github.com/su2code/TestCases.git ./TestData | |||
- git clone --depth=1 -b enhancement_fixed_cl https://github.com/su2code/TestCases.git ./TestData |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont forget to change that
*/ | ||
bool WriteVolume_Output(CConfig *config, unsigned long Iter) override; | ||
bool WriteVolume_Output(CConfig *config, unsigned long Iter, bool force_writing) override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with that change!
* \brief Write any additional output defined for the current solver. | ||
* \param[in] config - Definition of the particular problem per zone. | ||
*/ | ||
void SetAdditionalScreenOutput(CConfig *config) override; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea to add a routine like that.
su2double Cauchy_Value, /*!< \brief Summed value of the convergence indicator. */ | ||
Cauchy_Func; /*!< \brief Current value of the convergence indicator at one iteration. */ | ||
unsigned short Cauchy_Counter; /*!< \brief Number of elements of the Cauchy serial. */ | ||
su2double *Cauchy_Serie; /*!< \brief Complete Cauchy serial. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for cleaning that up
@@ -2820,10 +2820,11 @@ bool CFlowOutput::WriteVolume_Output(CConfig *config, unsigned long Iter){ | |||
return true; | |||
} | |||
} else { | |||
return ((Iter > 0) && Iter % config->GetVolume_Wrt_Freq() == 0); | |||
if (config->GetFixed_CL_Mode() && config->GetFinite_Difference_Mode()) return false; | |||
return ((Iter > 0) && Iter % config->GetVolume_Wrt_Freq() == 0) || force_writing; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am ok with that!
void CFlowCompOutput::SetAdditionalScreenOutput(CConfig *config){ | ||
|
||
if (config->GetFixed_CL_Mode()){ | ||
SetFixedCLScreenOutput(config); | ||
} | ||
} | ||
|
||
void CFlowCompOutput::SetFixedCLScreenOutput(CConfig *config){ | ||
PrintingToolbox::CTablePrinter FixedCLSummary(&cout); | ||
|
||
if (fabs(historyOutput_Map["CL_DRIVER_COMMAND"].value) > 1e-16){ | ||
FixedCLSummary.AddColumn("Fixed CL Mode", 40); | ||
FixedCLSummary.AddColumn("Value", 30); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep looks good
In the process of the final merge which involves pulling corresponding TestCase and Tutorial branches into their develops as well. (and changing the travis.yml file) |
Proposed Changes
This is a simple enhancement of the fixed CL mode. Currently the Fixed CL mode only exits when the specified iteration limit is reached. With these changes, the mode is a little more robust and checks for the specified Residual or Cauchy convergence and additionally checks that the CL is converged to the target CL value to within CAUCHY_EPS.
I have currently preserved the Finite Differencing that occurs at the end of the Fixed CL mode. I am not sure why this is required and would like to get rid of it if possible. If there is a reason for the finite differencing to calculate dCL/dAlpha, please let me know.
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.