-
Notifications
You must be signed in to change notification settings - Fork 247
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
[CoSimulationApplication] Adding AcuSolve
wrapper
#11512
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.
I recommend adding a test with a dummy solver, like I did in other tests
Otherwise this is entirely untested
else: | ||
cmd = "acuRun.bat -inp "+inputFile+" -pb "+problem+" -dir "+working_directory+" -pdir "+problem_directory+" -np "+str(np)+" -nt "+str(nt)+" -verbose "+str(echo_level)+" -lbuff" | ||
cs_tools.cs_print_info(self.name + ": " + cmd) | ||
subprocess.Popen(cmd) |
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.
subprocess.run is the newer and safer alternative
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.
Okay
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 was unused, replaced with os.system (common for Linux/windows)
self.vtk_output = VtkOutputProcess(self.model, vtk_output_configuration) | ||
self.vtk_output.ExecuteInitialize() | ||
self.vtk_output.ExecuteBeforeSolutionLoop() | ||
self.vtk_output.PrintOutput() |
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.
Unconditional?
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.
Implementation is not mine...
# Python exception in case not detected OS | ||
raise Exception("Unsupported operating system detected.") | ||
cs_tools.cs_print_info(self.name + ": " + cmd) | ||
os.system(cmd) |
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.
That is even worse 😅 (talking mainly about security issues, had sth similar at work)
I recommend using subprocess.run
Then it will be... Tomorrow
El lun., 28 ago. 2023 20:09, Philipp Bucher ***@***.***>
escribió:
… ***@***.**** commented on this pull request.
------------------------------
In
applications/CoSimulationApplication/python_scripts/solver_wrappers/external/acusolve_wrapper.py
<#11512 (comment)>
:
> + elif platform == "win32":
+ base_cmd = "acuRun.bat -inp "
+ buffer_cmd = " -lbuff"
+ if gpu != "FALSE":
+ cmd = base_cmd + common_cmd + " -gpu " + gpu + verbose_cmd + buffer_cmd
+ elif rst != "FALSE":
+ cmd = base_cmd + common_cmd + " -rst " + verbose_cmd + buffer_cmd
+ elif frst != "FALSE":
+ cmd = base_cmd + common_cmd + " -frst " + verbose_cmd + buffer_cmd
+ else:
+ cmd = base_cmd + common_cmd + verbose_cmd + buffer_cmd
+ else:
+ # Python exception in case not detected OS
+ raise Exception("Unsupported operating system detected.")
+ cs_tools.cs_print_info(self.name + ": " + cmd)
+ os.system(cmd)
That is even worse 😅 (talking mainly about security issues, had sth
similar at work)
I recommend using subprocess.run
—
Reply to this email directly, view it on GitHub
<#11512 (review)>,
or unsubscribe
</~https://github.com/notifications/unsubscribe-auth/AEYQZADGSZY5IMQKOSBOX6LXXTNGHANCNFSM6AAAAAA4BGCQBQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
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.
Nice
📝 Description
This PR introduces a new CoSimulation wrapper called
acuSolveWrapper
that serves as a dedicated Kratos wrapper for theacuSolve
solver.NOTE: This wrapper class was not developed by me, the details I can provide are limited.
The contents of the commit include the following changes:
acuSolveWrapper
class that extendsCoSimulationSolverWrapper
.Initialize
function for theacuSolveWrapper
class to perform additional initialization tasks, such as importing coupling interfaces and configuring VTK output settings.🆕 Changelog
AcuSolve
wrapper