AllenDowney / ThinkDSP

Think DSP: Digital Signal Processing in Python, by Allen B. Downey.
https://allendowney.github.io/ThinkDSP/
3.97k stars 3.23k forks source link

More NumPy, less home-grown classes #10

Closed mgeier closed 5 years ago

mgeier commented 8 years ago

I very much admire that you put all the sources of the book and all the code online, that's really great! I'm sure this is a great resource for many readers and hackers, as the stars and forks already indicate.

Since you stated in the README that this is "work in progress", I took the liberty of making a few comments:

Your thinkdsp module sure is well-intended, but I think it causes severe lock-in and it will, however convenient in the beginning, at some point be an obstacle for readers who learn Python with your book (which I hope there are many of!).

Your Wave class may seem like a good idea, but I honestly think it's not. It may seem convenient, but I think it's not as convenient as it's restricting.

It would be more straightforward and future-proof if you just handle your signals as NumPy arrays. All your functions returning signals should return NumPy arrays and instead of methods of the Wave class you should have free functions to manipulate the signals.

IMHO, this has the one disadvantage that you have to take care of the sampling rate by hand, but this is really something everyone using DSP should learn from the beginning. So even that may be construed as an advantage, since the sampling rate has to be handled explicitly instead of implicitly (and we all know what the Zen of Python says about that!).

Your Signal family of classes might be justified, but probably you'd be also better off just making functions that return NumPy arrays.

Finally, let me make a shameless plug for 2 libraries I've been working on:

https://github.com/bastibe/PySoundFile https://github.com/spatialaudio/python-sounddevice

Both are very easy to install via pip (on Windows/OSX/Linux) and they should give you more comprehensive functionality for reading/writing sound files and playing/recording sound, respectively. You should give them a try!

AllenDowney commented 8 years ago

Thanks for these thoughts. I think I understand your point, and there are definitely advantages to using raw NumPy rather than defining a new class. But I am inclined to stick with the classes for two reasons:

1) As you pointed out, a Wave object carries its framerate with it. And in the current version of the library, it also carries the timestamps of the samples. Similarly, a Spectrum contains the frequencies that correspond to the complex amplitudes. I think passing these containers around is substantially better than passing around the pieces separately (or recomputing them on demand).

2) I think the classes improve readability, which is crucial to the project. I'm using Python as an alternative to mathematical notation, but that only works if I can write super readable code. The abstractions provided by Wave and Spectrum are an important part of that.

mgeier commented 8 years ago

The problem with your custom Python classes is that you're creating a DSL which you are teaching to your students. They are actually learning your toy language instead of learning Python and how to use it for signal processing.

But if you want them to learn a real language which they can actually productively use after your course, then you should teach them Python + NumPy.

And if they are learning Python + NumPy anyway, your DSL causes additional cognitive load on top of that.

For comparison, here's how I tried to introduce digital signals in the context of Python without any custom "magic objects": http://nbviewer.jupyter.org/github/mgeier/python-audio/blob/master/simple-signals.ipynb

If you think this is too much for your students, you could make helper functions (that accept/return NumPy arrays) to hide some of the details (only to reveal them later in the course), but IMHO the abstractions shouldn't hide such important facts as the sampling rate.