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

Structural stress penalty function for optimization, fix some CConfig problems #1176

Merged
merged 11 commits into from
Jan 27, 2021

Conversation

pcarruscag
Copy link
Member

Proposed Changes

A differentiable maximum VM stress function for optimization.
Simplifies the "DoubleArray" option to avoid one array of default values and another of actual values.

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.

@pcarruscag pcarruscag added this to In progress in General Maintenance via automation Jan 22, 2021
@pr-triage pr-triage bot added the PR: draft label Jan 22, 2021
Omega = (config->GetRotation_Rate(3)/config->GetOmega_Ref());
Omega = config->GetRotation_Rate(2)/config->GetOmega_Ref();
Copy link
Member Author

Choose a reason for hiding this comment

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

Using static types lets the compiler find bugs for us at compile time :)
Using bare-pointer does not let the compiler do anything :(

Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious how the compiler found that now that the underlying Rotation_Rate is static because is has 3 elements initialized to 0. So how did the compiler know but I guess I miss sth here

Copy link
Member Author

Choose a reason for hiding this comment

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

Because this is not matlab or fortran xD

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh man am I blind... insert palmface here

Copy link
Member Author

Choose a reason for hiding this comment

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

As blind an any programmer, we should have XYZ enums

SetHistoryOutputValue("AVG_NORMALVEL", Tot_Surface_Enthalpy);
SetHistoryOutputValue("AVG_NORMALVEL", Tot_Surface_NormalVelocity);
Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @WallyMaier , I literally changed the line below 2 days ago and missed this.

@pcarruscag pcarruscag marked this pull request as ready for review January 22, 2021 19:40
Copy link
Contributor

@WallyMaier WallyMaier left a comment

Choose a reason for hiding this comment

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

This looks good to me. I appreciate the clean-up done. Especially getting rid of SU2_MSH!

@@ -1262,8 +1239,8 @@ void CConfig::SetConfig_Options() {
/*!\brief INC_DENSITY_INIT \n DESCRIPTION: Initial density for incompressible flows (1.2886 kg/m^3 by default) \ingroup Config*/
addDoubleOption("INC_DENSITY_INIT", Inc_Density_Init, 1.2886);
/*!\brief INC_VELOCITY_INIT \n DESCRIPTION: Initial velocity for incompressible flows (1.0,0,0 m/s by default) \ingroup Config*/
default_vel_inf[0] = 1.0; default_vel_inf[1] = 0.0; default_vel_inf[2] = 0.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the removal of all the default_

@pcarruscag pcarruscag merged commit c008fd4 into develop Jan 27, 2021
General Maintenance automation moved this from In progress to Done Jan 27, 2021
@pcarruscag pcarruscag deleted the stress_penalty branch January 27, 2021 15:50
Copy link
Contributor

@TobiKattmann TobiKattmann left a comment

Choose a reason for hiding this comment

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

Can't say much to the FEA stuff but thanks 💐 for the Config cleanup. Yet another net-code-reduction :)

@@ -117,6 +109,7 @@ class CConfig {
su2double Opt_RelaxFactor; /*!< \brief Scale factor for the line search. */
su2double Opt_LineSearch_Bound; /*!< \brief Bounds for the line search. */
su2double StartTime;
unsigned short SmoothNumGrid; /*!< \brief Smooth the numerical grid. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, what is the numerical grid (or is it just the mesh) and when is it smoothed?
greping through all cpp,hpp,inl of the code base didn't really enlighten me, just found some evidence in Cpoint

Copy link
Member Author

Choose a reason for hiding this comment

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

Left over from SU2_MSH I believe, there is some geometry routine that uses those variables of CPoint, but it is never called.

@@ -23,6 +23,7 @@ INC_DENSITY_MODEL= VARIABLE
INC_ENERGY_EQUATION = YES
INC_DENSITY_INIT= 0.00597782417156
INC_TEMPERATURE_INIT= 288.15
INC_VELOCITY_INIT= (0, 0, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you have to add this option here? I might have missed that in the code but if the option is not set I understood your changes that the default remains the same, but with out explicitly having containers for the default value? (default here would be 1,0,0)

Omega = (config->GetRotation_Rate(3)/config->GetOmega_Ref());
Omega = config->GetRotation_Rate(2)/config->GetOmega_Ref();
Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious how the compiler found that now that the underlying Rotation_Rate is static because is has 3 elements initialized to 0. So how did the compiler know but I guess I miss sth here

Comment on lines -2025 to +1998
default_vel_inf[0] = 0.0; default_vel_inf[1] = 0.0; default_vel_inf[2] = 0.0;
/* DESCRIPTION: Coordinates of the rigid motion origin */
addDoubleArrayOption("MOTION_ORIGIN", 3, Motion_Origin, default_vel_inf);
addDoubleArrayOption("MOTION_ORIGIN", 3, Motion_Origin);
Copy link
Member Author

Choose a reason for hiding this comment

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

@TobiKattmann the default velocity was not 1,0,0 because it was being re-used here as the default value for something else.

This trips a lot of people (myself included) but the actual parsing of the options does not take place in this method, it creates a list of option parsing objects but it does not read immediately. Hence the need for the config postprocessing method to check for conflicting things and so on.

Comment on lines +1242 to +1243
vel_init[0] = 1.0; vel_init[1] = 0.0; vel_init[2] = 0.0;
addDoubleArrayOption("INC_VELOCITY_INIT", 3, vel_init);
Copy link
Contributor

Choose a reason for hiding this comment

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

@pcarruscag Ok I see. This really seems so straight forward with what it replaces but it isn't

@rafapalacios
Copy link

rafapalacios commented Jan 28, 2021 via email

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

Successfully merging this pull request may close these issues.

None yet

4 participants