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

[CoSimulationApplication] Adding AcuSolve wrapper #11512

Merged
merged 3 commits into from
Aug 29, 2023

Conversation

loumalouomega
Copy link
Member

📝 Description

This PR introduces a new CoSimulation wrapper called acuSolveWrapper that serves as a dedicated Kratos wrapper for the acuSolve 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:

  1. Definition of the acuSolveWrapper class that extends CoSimulationSolverWrapper.
  2. Creation of model parts and associated variables within the Kratos environment using coupling data settings.
  3. Reading arguments from a JSON file, including application name, working directory, problem directory, region, echo level, input file, number of processors, number of threads, GPU usage, restart flags, and time increment.
  4. Launching the acuSolve solver based on the provided arguments using platform-specific commands.
  5. Definition of an Initialize function for the acuSolveWrapper class to perform additional initialization tasks, such as importing coupling interfaces and configuring VTK output settings.
  6. Importing of coupling interfaces based on model part names specified in settings.
  7. Configuring and initializing VTK output for visualization purposes.

🆕 Changelog

philbucher
philbucher previously approved these changes Aug 28, 2023
Copy link
Member

@philbucher philbucher left a 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)
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay

Copy link
Member Author

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()
Copy link
Member

Choose a reason for hiding this comment

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

Unconditional?

Copy link
Member Author

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)
Copy link
Member

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

@loumalouomega
Copy link
Member Author

loumalouomega commented Aug 28, 2023 via email

Copy link
Member

@philbucher philbucher left a comment

Choose a reason for hiding this comment

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

Nice

@loumalouomega loumalouomega merged commit f0b832f into master Aug 29, 2023
@loumalouomega loumalouomega deleted the cosim/adding-acusolve-wrapper branch August 29, 2023 10:29
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.

2 participants