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
added simulation for brownian motion #388
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.
Thank you for this new demo @SunTzunami!
See below for some comments. Overall, it would be good if you could reduce the number of actors.
docs/examples/viz_brownian_motion.py
Outdated
for i in range(num_particles): | ||
coor_e = coords[i] | ||
coords[i] = np.array(generatepath(coords[i], delta, time_step)) | ||
coordinates = np.array([[coords[i], coor_e]]) | ||
path_actor = actor.line(coordinates, colors[i], linewidth=3) | ||
scene.add(path_actor) |
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.
You create a lot of actors in this loop. This is memory-consuming and performance greedy. it would be better if you could create only 4 line actors and update the path of each actor. Can you update this point? thank you.
docs/examples/viz_brownian_motion.py
Outdated
############################################################################### | ||
# Creating a container (cube actor) inside which the particle(s) move around | ||
|
||
center1 = np.array([[0, 0, 0]]) |
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 variable is unnecessary, you can feed the point directly on your cube actor if you have only 1 center
docs/examples/viz_brownian_motion.py
Outdated
delta = 1.8 | ||
num_particles = 4 | ||
common_origin = np.array([0, 0, 0]) | ||
coords = [common_origin for i in range(num_particles)] |
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.
np.zeros((num_particles, 3))
instead
docs/examples/viz_brownian_motion.py
Outdated
num_particles = 4 | ||
common_origin = np.array([0, 0, 0]) | ||
coords = [common_origin for i in range(num_particles)] | ||
colors = [window.colors.red, window.colors.blue, window.colors.yellow, |
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, try to use NumPy array
docs/examples/viz_brownian_motion.py
Outdated
# Function that generates the path of the particles | ||
|
||
|
||
def generatepath(point, delta, time_step): |
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.
generate_path
instead
docs/examples/viz_brownian_motion.py
Outdated
|
||
|
||
def generatepath(point, delta, time_step): | ||
x, y, z = (point[i] for i in range(3)) |
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 did not check what is inside the point
variable. Why not x, y, z = point
?
Hi @skoudoro ! Thanks for the really insightful review! I made the requested changes. The animation is rendered much better now after using numpy arrays and after reducing the number of actors. PTAL. |
Hello @SunTzunami! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-03-10 10:15:23 UTC |
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.
Hi @SunTzunami,
Thank you for this simulation and all the updates. I need to play a bit with this tutorial and I will come back to you.
Thank you !
docs/examples/viz_brownian_motion.py
Outdated
self.colors = colors | ||
self.delta = delta | ||
self.num_total_steps = num_total_steps | ||
self.time_step = total_time/num_total_steps |
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.
Can you add space around the operator
docs/examples/viz_brownian_motion.py
Outdated
self.path_actor = actor.line([self.position], colors, linewidth=3) | ||
self.vertices = utils.vertices_from_actor(self.path_actor) | ||
self.vcolors = utils.colors_from_actor(self.path_actor, 'colors') | ||
self.no_vertices_per_point = len(self.vertices)/num_total_steps |
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.
Can you add space around the operator
docs/examples/viz_brownian_motion.py
Outdated
nvpp, axis=0) | ||
|
||
def update_path(self, counter_step): | ||
if(counter_step < self.num_total_steps): |
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.
parenthesis are not needed here
Hi @skoudoro ! Thanks for the review! I made the requested changes and tweaked the code a bit to add more functionality. PTAL. |
Thanks @SunTzunami! This is a nice Tutorial, merging |
Thanks @skoudoro ! Should I add a gif for the same to the communication assets? |
yes 👍🏾 |
I made a small simulation of 4 particles exhitbiting brownian motion using FURY.
Preview-
FURY.0.6.1.post164.dev0+g102c618.2021-02-27.17-38-38.mp4