-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature/proximal parallel #117
Conversation
python/rainbow/simulators/prox_soft_bodies/gauss_seidel_parallel.py
Outdated
Show resolved
Hide resolved
python/rainbow/simulators/prox_soft_bodies/gauss_seidel_parallel.py
Outdated
Show resolved
Hide resolved
python/rainbow/simulators/prox_soft_bodies/gauss_seidel_parallel.py
Outdated
Show resolved
Hide resolved
python/rainbow/simulators/prox_soft_bodies/gauss_seidel_parallel.py
Outdated
Show resolved
Hide resolved
Hi Kenny, Regarding the code redundancy, my idea is to create a new folder named "proximal" in the "rainbow." The visualization of the folder is shown as follows:
class BaseSolver:
def __init__(self, J, WJT, b, mu, friction_solver, engine):
...
def check_convergence(self):
...
def sweep(self):
# Subclasses implement specific calculation logic
pass
def solver(self):
...
for iteration in range(self.engine.params.max_iterations):
x = self.sweep(x)
self.check_convergence(x, sol, iteration)
...
return x, self.stats
class ParallelJacobiSolver(BaseSolver):
def sweep(self, x):
pass
class ParallelHybridSolver(BaseSolver):
def sweep(self, x):
pass
class ParallelGaussSeidelSolver(BaseSolver):
def sweep(self, x):
pass
class SerialGaussSeidelSolver(BaseSolver):
def sweep(self, x):
pass
... But I am not sure if this idea is ok or not. Could you give some comments on it ? |
Hi Dong, I agree that separating out the common functionality in an independent module/sub-package makes sense. I agree that inheritance appears to be a good design strategy to make sure all the different kinds of solvers comply with the expected interface. I perhaps feel that the new module should be placed in rainbow/simulators/proximal_contact/ Because it is "simulation solver" code and as such should be inside the simulator module. I would suggest the following renaming as well prox.py -> proximal_opertors.py I would further suggest that the building of the contact graph and the coloring of the graph be given its own module. Perhaps, blocking.py This way it will be easier to add documentation and units to keep things more isolated. It helps to isolate these details of the code as well (improved information hiding) Lastly, I would suggest making one convenience module for importing all solvers into a simulator. Like prox_solvers.py This is just so one can have a single import statement in any given simulator. import rainbow.simulators.proximal_contact.prox_solvers as CONTACT_SOLVERS I am fine with different wording as long as the meaning of the content is clear:-) |
Thanks for the feedback! I like your ideas, especially about renaming the files for clarity. I will go ahead with these changes. |
Hi Kenny, The latest update to the code has been committed, and changelogs are as follows:
|
Hi Kenny, I pushed the new code to this branch based on our last discussion.
|
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 job, I have a minor naming issue on "body_type" which I do not thing is 100% clear enough.
Hi Kenny, I used the |
Three parallel proximal scheme solvers for soft-body are supported: