-
Notifications
You must be signed in to change notification settings - Fork 915
Small update of the python fsi interface #1148
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
Conversation
pcarruscag
left a comment
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 think the tests run in a docker https://hub.docker.com/r/su2code/test-su2/tags?page=1&ordering=last_updated
Which is created based on https://github.com/su2code/Docker-Builds/blob/master/build/Dockerfile
(This is the first time I've tried to figure this out) using apt-get install will probably not work.
Perhaps @talbring can shed some light, and in the process document this a little.
TestCases/tutorials.py
Outdated
| testcase_su2_nastran = TestCase('py_su2_nastran') | ||
| testcase_su2_nastran.cfg_dir = "TestCases/py_su2_nastran" |
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 file is supposed to get the grids from the tutorials repo, not from testcases.
TestCases/py_su2_nastran/fsi.cfg
Outdated
| % Author: % | ||
| % Institution: % | ||
| % Date: % | ||
| % File Version 7.0.2 "Blackbird" % |
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.
version numbers
TestCases/py_su2_nastran/fluid.cfg
Outdated
| % Adaptive CFL number (NO, YES) | ||
| CFL_ADAPT= NO | ||
| % | ||
| % Parameters of the adaptive CFL number (factor down, factor up, CFL min value, | ||
| % CFL max value ) | ||
| CFL_ADAPT_PARAM= ( 1.5, 0.5, 1.0, 100.0 ) | ||
| % | ||
| % Runge-Kutta alpha coefficients | ||
| RK_ALPHA_COEFF= ( 0.66667, 0.66667, 1.000000 ) |
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.
Please remove irrelevant options such as these
TestCases/py_su2_nastran/fluid.cfg
Outdated
| MUSCL_FLOW= YES | ||
| % Slope limiter (VENKATAKRISHNAN, MINMOD) | ||
| SLOPE_LIMITER_FLOW= VENKATAKRISHNAN |
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.
these
TestCases/py_su2_nastran/fluid.cfg
Outdated
| % Spatial numerical order integration (1ST_ORDER, 2ND_ORDER, 2ND_ORDER_LIMITER) | ||
| % | ||
| MUSCL_TURB= NO | ||
| SLOPE_LIMITER_TURB= VENKATAKRISHNAN |
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.
and this.
Because it is a lot of work when we need to update things.
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.
Very sorry... I took the wrong template. Fixing it now
SU2_PY/SU2_Nastran/pysu2_nastran.py
Outdated
| print("\n------------------------------ Configuring the structural tester solver for FSI simulation ------------------------------") | ||
| print("\n---------- Configuring the structural tester solver for FSI simulation ----------") |
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.
It would be better to use rfill / lfill with the total width defined somewhere central.
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.
Great! Thank you for the suggestion
| if case(): | ||
| sys.exit('Imposed function {} not found, please implement it in pysu2_nastran.py'.format(self.tipo)) | ||
| break |
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.
Don't do this, throw an exception.
| if (TimeMarching == DT_STEPPING_2ND) cout << "Unsteady simulation, dual time stepping strategy (second order in time)."<< endl; | ||
| if (Unst_CFL != 0.0) cout << "Time step computed by the code. Unsteady CFL number: " << Unst_CFL <<"."<< endl; | ||
| else cout << "Unsteady time step provided by the user (s): "<< Delta_UnstTime << "." << endl; | ||
| cout << "Total number of internal Dual Time iterations: "<< InnerIter <<"." << endl; |
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.
Is this output not relevant?
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 think it is redundant as the number of inner iterations are displayed in any case. Plus, the number of inner iterations is actually nInnerIter, thus that output always display Total number of internal Dual Time iterations: 0
Is it maybe a legacy of old implementations?
|
The modification required should be implemented. @pcarruscag thank you very much for taking the time to give me the suggestions and for checking out the issue with docker. |
|
Dear all, I temporarily removed the test case because there were a couple of fix to do before. If you still think the test case for this feature may be important, I think a separate PR is more appropriate, as there will be the need to discuss about the best way to incorporate it. In the meantime, I included a fix on the outputs that allow to print grid velocities in the output files using the new mesh deformation solver and a small fix on the restart from the python side. Looking forward for your comments! |
pcarruscag
left a comment
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
Proposed Changes
Dear all, in this PR there are few modification related to the new fsi interface in python, namely:
Regarding the last point, I am having issues trying to understand how to appropriately set up the regression. In particular, the fsi interface needs two additional packages to run (libspatialindex and petsc) with their counterparts in python (rtree and petsc4py). @pcarruscag suggested me to add the apt-get lines in the regression.yml file, but I am really not sure where to place them and I do not want to mess it up.
Further, the files in the tutorial folder cannot be used for this scope. Indeed, in the tutorial, there are the cfg files that can be used to reproduce the same behaviour predicted by the theory of Theodorsen. However, in order to do so, some iterations of fluid only simulation must be performed before the coupling. This takes time so I prepared a test case where the two solvers (fluid and structure) are coupled from the beginning. Unfortunately, the mesh is ignored by git in the TestCase folder, so I am not sure how to make it run.
I would really appreciate if you could help me with this last point.
Related Work
#1124
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.