[JOSS Review] Code Examples Feedback

sea-bass commented 1 year ago

Thank you for submitting pyswarming to JOSS! As one of your reviewers, I'll collect a few issues here with the "getting started" code examples.

Gentler Introduction to `simulate()` vs. custom animation

The first few examples are super simple, and that is fantastic! But then it very quickly turns into a more complicated setup that requires custom animation functions, starting poses, etc. and I felt like I missed some explanation.

Based on my understanding (and maybe I am missing something): It might be worth saying that there is a "simple" method where you create a swarm and then call simulate() which handles a lot the work for simple behaviors. However, there is also a more custom workflow of defining your own animation function along with your behaviors, and then calling the matplotlib animation.FuncAnimation().

So my feedback here is to maybe document the general approaches a little better and give users some direction on when to use which approach.

Using named arguments

All the doc examples use this format:

my_swarm = sw.Swarm(10, # number of robots
                    0.5, # linear speed of each robot
                    1.0, # sampling time
                    [0.0, 0.0], # robots deployed randomly around x = 0.0, y = 0.0 (+- 5.0 meters)
                    [[-50.0, 50.0], [-50.0, 50.0]], # plot limits x_lim, y_lim
                    ['repulsion']) # list of behaviors

However, you are already using named arguments, and I could argue that this could be made clearer using something like this

my_swarm = sw.Swarm(n=10,  # number of robots
                    linear_speed=0.5,  # linear speed of each robot
                    dT=1.0,  # sampling time
                    deployment_point=[0.0, 0.0],  # robots deployed randomly around x = 0.0, y = 0.0 (+- 5.0 meters)
                    plot_limits=[[-50.0, 50.0], [-50.0, 50.0]],  # plot limits x_lim, y_lim
                    behaviors=["repulsion"],  # list of behaviors
)

I also have a question here... where does this +- 5.0 meters in the deployment_point come in? Might be good to explicitly define that in the examples as well, and say that the orientation distribution is similarly between 0 to 2*pi. See https://github.com/mrsonandrade/pyswarming/issues/1 for more details.

No globals

Instead of defining global r and then using a function called animate(), consider maybe passing r in as an input argument? Also, using single letter variables is often not very descriptive, you could call this robot_poses or something.

So, for example:

# define each robot (x, y, z) position
robots = np.asarray([[8., 8., 0.],
                     [-8., 8., 0.],
                     [8., -8., 0.],
                     [-8., -8., 0.]])

...

# animation function. This is called sequentially
def animate(robots, i):
    for r_ind in range(len(robots)):
        r_i = robots[r_ind]
        r_j = np.delete(robots, np.array([r_ind]), axis=0)
        robots[r_ind] += s_i*(ps.aggregation(r_i, r_j) + ps.repulsion(r_i, r_j, 5.0))
    robot1.set_data(robots[0][0], robots[0][1])
    robot2.set_data(robots[1][0], robots[1][1])
    robot3.set_data(robots[2][0], robots[2][1])
    robot4.set_data(robots[3][0], robots[3][1])
    return (robot1,robot2,robot3,robot4,)

# call the animator. blit=True means only re-draw the parts that 
# have changed.
anim = animation.FuncAnimation(fig, lambda i: animate(robots, i),
                               init_func=init, frames=720,
                               interval=1, blit=True)

Provide More Variety of Examples

Right now, there is just one Jupyter notebook in the repo's Examples directory, but it might be better if you split out multiple examples into different files so people can run just the parts they care about.

Also, it would be great if you could offer the examples in plain text (.py) files as well. For example, I wasn't able to get the matplotlib animations working in the Jupyter notebooks running directly in VSCode, and I'm too lazy as a user to go figure this out just to run some quick examples.

Small issues

The examples never actually ask users to import numpy as np but instead they directly start using np in the code. Just add a quick line at the beginning where needed.
The shorthand import pyswarming.behaviors as ps is a bit confusing to me... instead of ps, should this be pb or something? It also is not consistent with import pyswarming as sw. Consider making this a little more consistent.

https://github.com/openjournals/joss-reviews/issues/5647

mrsonandrade commented 1 year ago

Thank you for your feedback! I have worked on all the points:

Document the general approach a little better and give users some direction on when to use which approach;
You are already using named arguments, and I could argue that this could be made clearer;
No globals;
Avoid single-letter variables (I have noticed that I forgot to avoid that on the README, I'll do it now);
Provide More Variety of Examples;
Small issues.

sea-bass commented 1 year ago

Looks great -- thanks for doing this!

On the readthedocs page, when you are using simulate() there is no animation that appears. But I'm fine with that if there is a limitation in the Jupyter notebooks render.

mrsonandrade commented 1 year ago

Good! To manage that I have included an argument in simulate(), so, now they are able to appear on the readthedocs page 😃

mrsonandrade / pyswarming