rougier / from-python-to-numpy

An open-access book on numpy vectorization techniques, Nicolas P. Rougier, 2017
http://www.labri.fr/perso/nrougier/from-python-to-numpy
Other
2.03k stars 339 forks source link

4.4 Spatial vectorization/numpy implementation incorrect operators #91

Closed vladislavneon closed 4 years ago

vladislavneon commented 4 years ago

There are three blocks containing code for the three rules.

This is a suspicious line used for normalizing in all the blocks: target *= np.divide(target, norm, out=target, where=norm != 0)

I suppose it must be just = instead of *=. In my opinion this assignment is redundant, taking into account out argument of np.divide.

rougier commented 4 years ago

I also find the line extremely suspicious and I can't remember why I wrote it this way. Did you check how simulation differs after your #92 fix ?

vladislavneon commented 4 years ago

There are completely different lines in your source code for simulation! For example, these lines are from the book:

norm = np.sqrt((target*target).sum(axis=1)).reshape(n, 1)
target *= np.divide(target, norm, out=target, where=norm != 0)

target *= max_velocity

And these are corresponding lines from boid_numpy.py:

norm = np.sqrt((target*target).sum(axis=1)).reshape(n, 1)
target = max_velocity * np.divide(target, norm, out=target,
                                          where=norm != 0)

So I'm pretty sure that these multiplication signs are typos.

vladislavneon commented 4 years ago

Also in this section it's unclear which of created masks should be used in lines such as

target = np.dot(mask, velocity)/count.reshape(n, 1)

May be I should better create another issue for this? But in that case I don't feel confident enough to fix it myself.

rougier commented 4 years ago

Since the boid_numpy is working, I think this is the right version. Maybe I tried a simplified version for the book (and implemented an optimized one in the script or I forgot to update the book). Maybe we can take the script as reference.

Ok to have a separate issue for the mask problem and we can work together to fix it (can you open an issue?).