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

Add python interface for setting AoA and AoS #2045

Merged
merged 6 commits into from
Jun 7, 2023
Merged

Add python interface for setting AoA and AoS #2045

merged 6 commits into from
Jun 7, 2023

Conversation

kursatyurt
Copy link
Contributor

Add Python interface for setting AoA and AoS for compressible flow simulations.

@patelha57
Copy link
Contributor

This is already addressed in PR #1750 in python_wrapper_structure.cpp

@pcarruscag
Copy link
Member

Thanks @patelha57 but that PR has 10k lines so I doubt it will be ready soon, the right way to introduce features is via small PRs such as this one.

@kursatyurt
Copy link
Contributor Author

@patelha57, I want to express my gratitude for your contribution. Your efforts are truly appreciated. However, I wanted to share some of my observations regarding the implementation, with all due respect.

In my humble opinion (IMHO), there seem to be a few flaws in the current implementation, particularly when it comes to setting the Mach number and Reynolds number. It appears that SU2 offers different fluid models, and in order to access the speed of sound for the far field, the following line of code may be more appropriate:

  Mach2Vel_FreeStream = auxFluidModel->GetSoundSpeed();

rather than using:

 su2double SoundSpeed = sqrt(Gamma * Gas_Constant * Temperature);

Perhaps these concerns can be addressed in the review of your PR. Similarly, I noticed that changing the Reynolds number doesn't seem to have any effect. It should modify certain variables, which are likely addressed in this section of the code: link to the relevant code. Additionally, it would be helpful to include a check to warn the user if the initialization is based on thermodynamic quantities.

Once again, I want to express my deep appreciation for your contributions.

SU2_CFD/include/solvers/CFVMFlowSolverBase.hpp Outdated Show resolved Hide resolved
SU2_CFD/include/solvers/CSolver.hpp Outdated Show resolved Hide resolved
SU2_CFD/include/solvers/CFVMFlowSolverBase.hpp Outdated Show resolved Hide resolved
SU2_CFD/include/solvers/CSolver.hpp Outdated Show resolved Hide resolved
SU2_CFD/src/python_wrapper_structure.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/python_wrapper_structure.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/python_wrapper_structure.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/python_wrapper_structure.cpp Outdated Show resolved Hide resolved
SU2_CFD/src/solvers/CEulerSolver.cpp Outdated Show resolved Hide resolved
@kursatyurt kursatyurt merged commit 35389df into su2code:develop Jun 7, 2023
@kursatyurt kursatyurt deleted the python_aoa_setter branch June 7, 2023 07:50
@patelha57
Copy link
Contributor

@patelha57, I want to express my gratitude for your contribution. Your efforts are truly appreciated. However, I wanted to share some of my observations regarding the implementation, with all due respect.

In my humble opinion (IMHO), there seem to be a few flaws in the current implementation, particularly when it comes to setting the Mach number and Reynolds number. It appears that SU2 offers different fluid models, and in order to access the speed of sound for the far field, the following line of code may be more appropriate:

  Mach2Vel_FreeStream = auxFluidModel->GetSoundSpeed();

rather than using:

 su2double SoundSpeed = sqrt(Gamma * Gas_Constant * Temperature);

Perhaps these concerns can be addressed in the review of your PR. Similarly, I noticed that changing the Reynolds number doesn't seem to have any effect. It should modify certain variables, which are likely addressed in this section of the code: link to the relevant code. Additionally, it would be helpful to include a check to warn the user if the initialization is based on thermodynamic quantities.

Once again, I want to express my deep appreciation for your contributions.

Hey @kursatyurt, thanks for the input! I absolutely agree about the speed of sound change that you suggested, so I will address it in my PR. I will also double check about the Reynolds number update. Haven't used that function before so not sure what the issue is.

It would be great to get additional input on that PR so please review if you get the chance.

@kursatyurt
Copy link
Contributor Author

It would be great to get additional input on that PR so please review if you get the chance.

@patelha57 I will take a look if I can find time, I have no experience with Adjoint methods probably I cannot provide any useful input there.

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.

3 participants