Update of SU2-Python 3.0 #1363
Update of SU2-Python 3.0 #1363
Conversation
There was a problem hiding this comment.
No need to be sorry for separate PR's @Nicola-Fonzi . Generally merging smaller sized chunks is better for everyone (although there is a limit at some point ;) ).
Just a few comments. But be aware: I am neither any knowledgable in python or CSD 🐌
SU2_PY/FSI_tools/FSI_config.py
Outdated
|
|
||
| if self._ConfigContent["MAPPING_MODES"] == "YES" and self._ConfigContent["CSD_SOLVER"]!="NATIVE": | ||
| self._ConfigContent["CSD_SOLVER"]="NATIVE" | ||
| print("WARNING: It has been requested to map modes, but the native solver was not selected. It is being selected automatically") |
There was a problem hiding this comment.
Concerning these screen-output-warnings: I personally think errors are the best warnings :) . Warnings disappear in the overall screen-output like hide-and-seek-champions. Setting or altering options based on the other (or because they were not set) can be dangerous. One thinks the config is the one ruling them all whilst in fact has no power over here at the code.
So instead
altering an option: error and say what should be done by the user or why that combination doesn't make sense
setting an option: error and require the user to make a choice
There are moments where that kind of stuff is reasonable, so that is up to you whether you error or warn. Maybe erroring helps you down the road as well. E.g. you actually wanted to set MAPPING_MODES to Yes and simply lost track of commented in/out options during debugging and then you wonder why the results are the way they are (that is of course a totally made up example ;) ).
There was a problem hiding this comment.
You are right. I changed the print in raise exception. Please note that the very first two cases are actually just default values for non-compulsory keywords. I will add a proper parser at a certain point...
| if CSD_Solver in ["NATIVE"]: | ||
| if myid == rootProcess: | ||
| print("\n") | ||
| print(" Initializing solid solver ".center(80,"*")) | ||
| if CSD_Solver == 'NATIVE': |
There was a problem hiding this comment.
With my first graders python knowledge: Arent line 117 and 121 equivalent/redundant right now? I guess in the list in line 117 you would incorporate more solvers in the future?
There was a problem hiding this comment.
Yes this is redundant but ready for other solvers to be added. 117 will be a list of supported serial solvers, then you have to import a different module for each of them. In this case there is only the one in 121
SU2_PY/fsi_computation.py
Outdated
| @@ -113,22 +113,22 @@ def main(): | |||
| comm.barrier() | |||
|
|
|||
| # --- Initialize the solid solver --- # (!! for now we are using only serial solid solvers) | |||
There was a problem hiding this comment.
Please transform those kinds of intermediate comments into serious ones for PR's (although this was not changed/done in this PR)
| # --- Initialize the solid solver --- # (!! for now we are using only serial solid solvers) | |
| # --- Initialize the solid solver --- # | |
| # --- For now, only serial solid solvers are used. --- # |
There was a problem hiding this comment.
yes, I sometimes forget about them. Sorry about that
SU2_PY/fsi_computation.py
Outdated
| # Parallel solvers | ||
| else: | ||
| SolidSolver = None | ||
| raise Exception('\n Invalid solid solver option') |
Proposed Changes
Sorry for the separate PR... we are working to include more solid solvers that can be used with the interface. This PR introduces few modifications that are required to include other solvers.
PR Checklist