Pull request

[Todd]

Hello,

I submitted a pull request #2522 [1]. It includes support for more basic spectrum plots like magnitude and phase spectrums. These are extremely commonly used in signal processing, acoustics, and many other fields, but are also very important for educational usage since pretty much anyone going through one of several engineering degrees like electrical engineering has to learn these types of plots. I have heard a number of colleagues complaining about the lack of these plots in matlab.

It has been up for 5 days, but I haven’t received any comments on its content or structure. I understand that it is a pretty substantial patch, but I think the features are very useful.

I am also a bit concerned that patches covering the same code might be submitted and accepted while I am waiting for this, since such changes would be hard to merge into my branch.

So does anyone have any thoughts on the patch?

[1] https://github.com/matplotlib/matplotlib/pull/2522

[Pierre_Haessig1]

Hi,

I submitted a pull request #2522 [1]. It includes support for more
basic spectrum plots like magnitude and phase spectrums. These are
extremely commonly used in signal processing, acoustics, and many
other fields, but are also very important for educational usage since
pretty much anyone going through one of several engineering degrees
like electrical engineering has to learn these types of plots. I have
heard a number of colleagues complaining about the lack of these plots
in matlab.

Having specific signal processing functions is indeed important. I hust
have a question about "phase" vs. "angle" spectrum. From browsing
quickly through you doc, it seems that the difference is just about
*unwrapping the phase*. If that's indeed the case, I've two
questions/remarks:

1) is the terminology "phase" vs. "angle" spectrum standardized ? I must
say I've never heard of one meaning "wrapped" and the other "unwrapped".
I didn't find similar terms in Matlab docs, but I didn't search that
thoroughly.

2) Should there be two separate functions for these two, or just one
function, with a switch argument `unwrap` ? (I guess it would be True by
default)

best,
Pierre

···

Le 20/10/2013 09:45, Todd a écrit :

[Todd]

Hi,

> I submitted a pull request #2522 [1]. It includes support for more
> basic spectrum plots like magnitude and phase spectrums. These are
> extremely commonly used in signal processing, acoustics, and many
> other fields, but are also very important for educational usage since
> pretty much anyone going through one of several engineering degrees
> like electrical engineering has to learn these types of plots. I have
> heard a number of colleagues complaining about the lack of these plots
> in matlab.

Having specific signal processing functions is indeed important. I hust
have a question about "phase" vs. "angle" spectrum. From browsing
quickly through you doc, it seems that the difference is just about
*unwrapping the phase*. If that's indeed the case, I've two
questions/remarks:

1) is the terminology "phase" vs. "angle" spectrum standardized ? I must
say I've never heard of one meaning "wrapped" and the other "unwrapped".
I didn't find similar terms in Matlab docs, but I didn't search that
thoroughly.

The "angle" function in numpy returns the wrapped angle, while the "unwrap"
function documentation talks about phase, so it is consistent with the
usage in numpy. Further, in signal processing, phases can have any value,
while "angle" often refers to the angle between two lines, which must be
wrapped.

There may be some ambiguity, but I made sure to explain it in the
documentation and provide links between the two functions so people know
what they should do if they want to use the other approach.

2) Should there be two separate functions for these two, or just one
function, with a switch argument `unwrap` ? (I guess it would be True by
default)

I originally was going to do that, but decided against it. The problem is
with specgram. Here, I thought it would be needlessly complicated to add
an "unwrap" parameter that is only useful for one "mode". To make it
obvious to users, I wanted to keep specgram as similar as possible to the
other plot types, and that involved keeping the parameter.

Further, this approach is simpler to code and easier to maintain. Having to
deal with the "unwrap" parameter would have been more difficult to
program. Dealing with both an "unwrap" parameter in some cases and a
separate "mode" in others would have been even more complicated.

Further, _spectral_helper and specgram already have a huge number of
arguments. This way I was able to get away with just adding one new
argument rather than two.

···

On Mon, Oct 21, 2013 at 3:13 PM, Pierre Haessig <pierre.haessig@...804...>wrote:

Le 20/10/2013 09:45, Todd a écrit :

[Todd]

I do have one question about my pull request.

Currently, both axes.psd and axes.csd return the same thing as mlab.psd and
mlab.csd, namely the spectrum and frequency points. They do NOT return the
line object that was plotted. This is different than specgram, which
returns the AxesImage object that was plotted in addition to the
mlab.specgram return values.

I think having access to the line object is important, so
axes.magnitude_spectrum, axes.angle_spectrum, and axes.phase_spectrum do
return the line object. I know this is inconsistent, but I think it is
very important and would strongly objefct to removing this.

The question, then, is whether this would be a good opportunity to add the
line object to the returned values for axes.psd and axes.csd. This would
be an API break, but would be very easy to fix, and it may be beneficial
enough to warrant it. What does everyone think?

···

On Sun, Oct 20, 2013 at 9:45 AM, Todd <toddrjen@...149...> wrote:

Hello,

I submitted a pull request #2522 [1]. It includes support for more basic
spectrum plots like magnitude and phase spectrums. These are extremely
commonly used in signal processing, acoustics, and many other fields, but
are also very important for educational usage since pretty much anyone
going through one of several engineering degrees like electrical
engineering has to learn these types of plots. I have heard a number of
colleagues complaining about the lack of these plots in matlab.

It has been up for 5 days, but I haven't received any comments on its
content or structure. I understand that it is a pretty substantial patch,
but I think the features are very useful.

I am also a bit concerned that patches covering the same code might be
submitted and accepted while I am waiting for this, since such changes
would be hard to merge into my branch.

So does anyone have any thoughts on the patch?

[1] Add additional spectrum-related plots and improve underlying structure by toddrjen · Pull Request #2522 · matplotlib/matplotlib · GitHub

[Pierre_Haessig1]

Hi,

Currently, both axes.psd and axes.csd return the same thing as
mlab.psd and mlab.csd, namely the spectrum and frequency points. They
do NOT return the line object that was plotted. This is different
than specgram, which returns the AxesImage object that was plotted in
addition to the mlab.specgram return values.

I think having access to the line object is important, so
axes.magnitude_spectrum, axes.angle_spectrum, and axes.phase_spectrum
do return the line object. I know this is inconsistent, but I think
it is very important and would strongly objefct to removing this.

The question, then, is whether this would be a good opportunity to add
the line object to the returned values for axes.psd and axes.csd.
This would be an API break, but would be very easy to fix, and it may
be beneficial enough to warrant it. What does everyone think?

I agree that it may be nice to have plt.psd/csd to return lines just
like plt.plot for increased consistency. I guess that returning the
values comes from Matlab style inspiration.

Maybe breaking the API is too strong. In that case, adding a
transitional keyword (for example `return_line=False`) to psd/csd may
help introduce your new proposed behavior in a softer manner. What do
you think ?

Of course, for your new functions, there is no API breakage problem.

best,
Pierre

···

Le 22/10/2013 12:31, Todd a écrit :

[Pierre_Haessig1]

Hi,

Thanks for the feedback. I agree that your documentation does make

clear the distinction between “phase” and “angle” and that it has a
consistency. I just feel that this distinction does not exist
“outside” …

But beyond this question of phase vs. angle, I must say I don't see

that big a use case for phase/angle spectrums[*] (as opposed to
magnitude which are much used). Also, in many cases, “spectrum” is
synonymous with spectral density, which implies “magnitude”. In the
end I wonder whether the notion of phase even makes sense for a
spectrogram ?

That's the reason why I'm a bit skeptical with the many new "mode

switches" in the spec helper, along with the new phase/angle_*
functions.

best,

Pierre

[*] On the other hand I do see a usecase of magnitude and phase for

plotting transfer functions (i.e. Bode diagrams). Those are not fft
based plots, so it’s a different topic I guess. Bode/Nyquist/Black
diagrams could be nice to have.

···

On Mon, Oct 21, 2013 at 3:13 PM,
Pierre Haessig <pierre.haessig@…804…>
wrote:

          1) is the terminology "phase" vs. "angle" spectrum

standardized ? I must

          say I've never heard of one meaning "wrapped" and the

other “unwrapped”.

          I didn't find similar terms in Matlab docs, but I didn't

search that

          thoroughly.

          The "angle" function in numpy returns the wrapped

angle, while the “unwrap” function documentation talks
about phase, so it is consistent with the usage in numpy.
Further, in signal processing, phases can have any
value, while “angle” often refers to the angle between two
lines, which must be wrapped.

          There may be some ambiguity, but I made sure to explain

it in the documentation and provide links between the two
functions so people know what they should do if they want
to use the other approach.

          2) Should there be two separate functions for these two,

or just one

          function, with a switch argument `unwrap` ? (I guess it

would be True by

          default)

      I originally was going to do that, but

decided against it. The problem is with specgram. Here, I
thought it would be needlessly complicated to add an “unwrap”
parameter that is only useful for one “mode”. To make it
obvious to users, I wanted to keep specgram as similar as
possible to the other plot types, and that involved keeping
the parameter.

      Further, this approach is simpler to code and easier to

maintain. Having to deal with the “unwrap” parameter would
have been more difficult to program. Dealing with both an
“unwrap” parameter in some cases and a separate “mode” in
others would have been even more complicated.

      Further, _spectral_helper and specgram already have a huge

number of arguments. This way I was able to get away with
just adding one new argument rather than two.

[Todd]

I considered that solution as well, but it seemed ugly. However, I think
having it available is important, so I will add it. The current behavior
can probably be deprecated at some point, allowing for a smoother
transition.

···

On Tue, Oct 22, 2013 at 5:41 PM, Pierre Haessig <pierre.haessig@...804...>wrote:

Hi,

Le 22/10/2013 12:31, Todd a écrit :
> Currently, both axes.psd and axes.csd return the same thing as
> mlab.psd and mlab.csd, namely the spectrum and frequency points. They
> do NOT return the line object that was plotted. This is different
> than specgram, which returns the AxesImage object that was plotted in
> addition to the mlab.specgram return values.
>
> I think having access to the line object is important, so
> axes.magnitude_spectrum, axes.angle_spectrum, and axes.phase_spectrum
> do return the line object. I know this is inconsistent, but I think
> it is very important and would strongly objefct to removing this.
>
> The question, then, is whether this would be a good opportunity to add
> the line object to the returned values for axes.psd and axes.csd.
> This would be an API break, but would be very easy to fix, and it may
> be beneficial enough to warrant it. What does everyone think?

I agree that it may be nice to have plt.psd/csd to return lines just
like plt.plot for increased consistency. I guess that returning the
values comes from Matlab style inspiration.

Maybe breaking the API is too strong. In that case, adding a
transitional keyword (for example `return_line=False`) to psd/csd may
help introduce your new proposed behavior in a softer manner. What do
you think ?

Of course, for your new functions, there is no API breakage problem.

best,
Pierre

[Todd]

Hi,

   1) is the terminology "phase" vs. "angle" spectrum standardized ? I

must
say I've never heard of one meaning "wrapped" and the other "unwrapped".
I didn't find similar terms in Matlab docs, but I didn't search that
thoroughly.

The "angle" function in numpy returns the wrapped angle, while the
"unwrap" function documentation talks about phase, so it is consistent with
the usage in numpy. Further, in signal processing, phases can have any
value, while "angle" often refers to the angle between two lines, which
must be wrapped.

There may be some ambiguity, but I made sure to explain it in the
documentation and provide links between the two functions so people know
what they should do if they want to use the other approach.

2) Should there be two separate functions for these two, or just one
function, with a switch argument `unwrap` ? (I guess it would be True by
default)

I originally was going to do that, but decided against it. The problem
is with specgram. Here, I thought it would be needlessly complicated to
add an "unwrap" parameter that is only useful for one "mode". To make it
obvious to users, I wanted to keep specgram as similar as possible to the
other plot types, and that involved keeping the parameter.

Further, this approach is simpler to code and easier to maintain. Having
to deal with the "unwrap" parameter would have been more difficult to
program. Dealing with both an "unwrap" parameter in some cases and a
separate "mode" in others would have been even more complicated.

Further, _spectral_helper and specgram already have a huge number of
arguments. This way I was able to get away with just adding one new
argument rather than two.

Thanks for the feedback. I agree that your documentation does make clear
the distinction between "phase" and "angle" and that it has a consistency.
I just feel that this distinction does not exist "outside" ...

But beyond this question of phase vs. angle, I must say I don't see that
big a use case for phase/angle spectrums[*] (as opposed to magnitude which
are much used).

I personally use phase and angle spectrums a huge amount. In signal
processing it is extremely important. It is a critical component in
acoustics. It is also used a lot to separate out signals that have been
mixed together. Knowing the phases of signals can also be very important
in certain optics applications and for radio signals and RADAR. Changes in
the phase spectrum over time (like you would get from a phase spectrogram)
is important for doppler analysis both with optical and acoustic signals.

Also, from an educational perspective, anyone taking a digital signal
processing course will need to produce magnitude/phase plots, probably both
with and without wrapping (since any decent digital signal processing
course will teach you about the pitfalls that occur due to phase
wrapping). So this will make matplotlib much more useful for such courses.

Also, in many cases, "spectrum" is synonymous with spectral density, which
implies "magnitude". In the end I wonder whether the notion of phase even
makes sense for a spectrogram ?

Yes, particularly electrical engineering. But there are many other fields
where spectral density is rarely used, and where more "ordinary" magnitude
and phase plots are the norm, as I explained in the previous paragraphs.

···

On Tue, Oct 22, 2013 at 6:06 PM, Pierre Haessig <pierre.haessig@...804...>wrote:

Le 21/10/2013 15:58, Todd a écrit :
On Mon, Oct 21, 2013 at 3:13 PM, Pierre Haessig <pierre.haessig@...804... > > wrote:

[Pierre_Haessig1]

Hi,

      Thanks for the feedback. I

agree that your documentation does make clear the distinction
between “phase” and “angle” and that it has a consistency. I
just feel that this distinction does not exist “outside” …

      But beyond this question of phase vs. angle, I must say I

don’t see that big a use case for phase/angle spectrums[*] (as
opposed to magnitude which are much used).

    I personally use phase and angle spectrums a huge amount.  In

signal processing it is extremely important. It is a critical
component in acoustics. It is also used a lot to separate out
signals that have been mixed together. Knowing the phases of
signals can also be very important in certain optics
applications and for radio signals and RADAR. Changes in the
phase spectrum over time (like you would get from a phase
spectrogram) is important for doppler analysis both with optical
and acoustic signals.

    Also, from an educational perspective, anyone taking a

digital signal processing course will need to produce
magnitude/phase plots, probably both with and without wrapping
(since any decent digital signal processing course will teach
you about the pitfalls that occur due to phase wrapping). So
this will make matplotlib much more useful for such courses.

      Also, in many cases,

“spectrum” is synonymous with spectral density, which implies
“magnitude”. In the end I wonder whether the notion of phase
even makes sense for a spectrogram ?

    Yes, particularly electrical engineering.  But there are many

other fields where spectral density is rarely used, and where
more “ordinary” magnitude and phase plots are the norm, as I
explained in the previous paragraphs.

Thanks for dealing with my ignorance. It's true that I have a biased

view on these frequency functions, because I mostly deal with random
signals these days.

In fact I'd like to understand a bit more how phase spectorgram

works. Since the signal must be cut into chunks to make the plot
along time, how is the phase computations “synchronised”, that is,
how does it use a common time reference. (because I would imagine
that otherwise the phase would make “jumps” between each window
frame). Do you have a pointer for how this is solved ? (or is this
problem just non-existing?).

best,

Pierre

···

Le 22/10/2013 19:14, Todd a écrit :

[Pierre_Haessig1]

Hello,

Now that that PR #2522 is merged, I don't know how much futher

commenting is useful, but I think there are two API details that I
feel could be better :

1) API dissymetry

The new pyplot/axes API is now:

 * 1 function **spectgram** now uses a mode argument to tune

this behavior :

*mode*: [ 'default' | 'psd' | 'magnitude' | 'angle' | 'phase' ]
             What sort of spectrum to use.  Default is 'psd'. which

takes
the power spectral density. ‘complex’ returns the
complex-valued
frequency spectrum. ‘magnitude’ returns the magnitude
spectrum.
‘angle’ returns the phase spectrum without unwrapping.
‘phase’
returns the phase spectrum with unwrapping.

* 3 new functions **      phase_spectrum, angle_spectrum,

magnitude_spectrum** which complement the exising psd/csd

-> I think this contributes to overcrowding axes/pyplot API. I

like much better what is done with specgram so I would propose to
add just one new function, for example spectrum (…mode=‘magnitude/angle/phase’).
Using the same mode keyword as for specgram would increase
the coherence of the API.

2) default NFFT value being hidden from views

`used to be def specgram(x, NFFT=256,  Fs=2, ...
    now is     def specgram(x, NFFT=None, Fs=None`

I think that NFFT is an important parameter of the spectrum

computation. It should not be * hidden from the immediate view of
the user* . The fact it is set to 256 is an arbitrary choice and
I think it even teaches something to the user (if he doesn’t know
what it is). Such a fixed value is an “invitation to change it”. Now
with NFFT=None, it is more likely to imply “a good choice was made
for you”, which is not true (because there is no such good choice
for all datasets).

best,

Pierre

[Pierre_Haessig1]

Hi,

···

        2) Should there be two separate functions for these two, or

just one

        function, with a switch argument `unwrap` ? (I guess it

would be True by

        default)

    I originally was going to do that, but

decided against it. The problem is with specgram. Here, I
thought it would be needlessly complicated to add an “unwrap”
parameter that is only useful for one “mode”. To make it
obvious to users, I wanted to keep specgram as similar as
possible to the other plot types, and that involved keeping the
parameter.

    Further, this approach is simpler to code and easier to

maintain. Having to deal with the “unwrap” parameter would have
been more difficult to program. Dealing with both an “unwrap”
parameter in some cases and a separate “mode” in others would
have been even more complicated.

    Further, _spectral_helper and specgram already have a huge

number of arguments. This way I was able to get away with just
adding one new argument rather than two.

[Pierre_Haessig1]

My, mistake, this 2nd point is pointless!

I got confused because I'm not familiar with the docstring

convention which make signatures are hard-written in the docstring.
Thanks Todd for pointing me this.

best,

Pierre

···

Le 25/10/2013 14:57, Pierre Haessig a écrit :

  2)

default NFFT value being hidden from views

  `used to be def specgram(x, NFFT=256,  Fs=2, ...
      now is     def specgram(x, NFFT=None, Fs=None`

  I think that NFFT is an important parameter of the spectrum

computation. It should not be * hidden from the immediate view
of the user* . The fact it is set to 256 is an arbitrary
choice and I think it even teaches something to the user (if he
doesn’t know what it is). Such a fixed value is an “invitation to
change it”. Now with NFFT=None, it is more likely to imply “a good
choice was made for you”, which is not true (because there is no
such good choice for all datasets).

[Todd]

I considering that. The problem is that people have to type all that.
"magnitude_spectrum" is long enough as it is, but
"unwrapped_phase_spectrum" is just a huge function name (24 characters). I
wanted something more concise. I think the documentation makes it clear
enough. I don't think we lose that much, the users only have to read the
docstring once, and they will avoid a lot of annoyance typing out such a
huge function name over and over.

···

On Fri, Oct 25, 2013 at 3:06 PM, Pierre Haessig <pierre.haessig@...804...>wrote:

Hi,

Le 21/10/2013 15:58, Todd a écrit :

  2) Should there be two separate functions for these two, or just one

function, with a switch argument `unwrap` ? (I guess it would be True by
default)

I originally was going to do that, but decided against it. The problem
is with specgram. Here, I thought it would be needlessly complicated to
add an "unwrap" parameter that is only useful for one "mode". To make it
obvious to users, I wanted to keep specgram as similar as possible to the
other plot types, and that involved keeping the parameter.

Further, this approach is simpler to code and easier to maintain. Having
to deal with the "unwrap" parameter would have been more difficult to
program. Dealing with both an "unwrap" parameter in some cases and a
separate "mode" in others would have been even more complicated.

Further, _spectral_helper and specgram already have a huge number of
arguments. This way I was able to get away with just adding one new
argument rather than two.

You've convinced me. I didn't have the big picture of your PR when writing
my previous messages. I like the approach you took for specgram, which put
"magnitude", "phase", "angle" on the same level. This indeed reduce the
number of keywords.

Coming back to the readability, what do you think of replacing "phase",
"angle" by "unwrapped phase", "phase". Beyond readability, it also
emphasizes for the reader the idea of "postprocessing" to get the unwrapped
phase, i.e. a something that can have it's issue.

[Todd]

Although it may reduce the number of functions, I don't think it increases
the coherence of the API. Quite the opposite, in fact. Besides the
inconsistency with psd and csd, I think having the plot types separate
makes sense because they really are not the same plots, they plot different
things in different ways and different units and using parameters.
Specgram, on the other hand, plots things in the same way with similar
units and mostly the same paramaters. So I think specgram plots group
together much more cleanly than the other spectrum-related plots.

···

On Fri, Oct 25, 2013 at 2:57 PM, Pierre Haessig <pierre.haessig@...804...>wrote:

Hello,

Now that that PR #2522 is merged, I don't know how much futher commenting
is useful, but I think there are two API details that I feel could be
better :

1) API dissymetry

The new pyplot/axes API is now:

* 1 function *spectgram* now uses a mode argument to tune this behavior :

*mode*: [ 'default' | 'psd' | 'magnitude' | 'angle' | 'phase' ]
             What sort of spectrum to use. Default is 'psd'. which takes
             the power spectral density. 'complex' returns the
complex-valued
             frequency spectrum. 'magnitude' returns the magnitude
spectrum.
            'angle' returns the phase spectrum without unwrapping. 'phase'
             returns the phase spectrum with unwrapping.

* 3 new functions *phase_spectrum, angle_spectrum, magnitude_spectrum*which complement the exising psd/csd

-> I think this contributes to overcrowding axes/pyplot API. I like much
better what is done with specgram so I would propose to add just one new
function, for example *spectrum*(...mode='magnitude/angle/phase'). Using
the same *mode *keyword as for specgram would increase the coherence of
the API

[Todd]

This could certainly be an issue, and no it isn't dealt with (nor am I
aware of a way it could be dealt with).

There are really several different questions here.

First, is it worthwhile having 1-D phase and angle spectrums
(phase_spectrum and angle_spectrum). I would argue that this is definitely
the case, as I already explained.

Second, is it worthwhile adding these to specgram? Frankly. probably not.
They have some robustness issues.

Third, given that implementing phase_spectrum and angle_spectrum
automatically gets us phase and angle specgrams, and that it would actually
take more code to turn them off than to leave them in, is there any reason
to explicitly disable these plot type? I would say no. It is a tool. It
may not be useful to very many people, and the people who do use it may
need to be careful to use it properly. But since we get it for free
anyway, I don't see a good reason to put in the extra code to remove
functionality that may be useful to someone but hurts no one.

···

On Fri, Oct 25, 2013 at 2:34 PM, Pierre Haessig <pierre.haessig@...804...>wrote:

Hi,

Le 22/10/2013 19:14, Todd a écrit :

Thanks for the feedback. I agree that your documentation does make clear

the distinction between "phase" and "angle" and that it has a consistency.
I just feel that this distinction does not exist "outside" ...

But beyond this question of phase vs. angle, I must say I don't see that
big a use case for phase/angle spectrums[*] (as opposed to magnitude which
are much used).

I personally use phase and angle spectrums a huge amount. In signal
processing it is extremely important. It is a critical component in
acoustics. It is also used a lot to separate out signals that have been
mixed together. Knowing the phases of signals can also be very important
in certain optics applications and for radio signals and RADAR. Changes in
the phase spectrum over time (like you would get from a phase spectrogram)
is important for doppler analysis both with optical and acoustic signals.

Also, from an educational perspective, anyone taking a digital signal
processing course will need to produce magnitude/phase plots, probably both
with and without wrapping (since any decent digital signal processing
course will teach you about the pitfalls that occur due to phase
wrapping). So this will make matplotlib much more useful for such courses.

Also, in many cases, "spectrum" is synonymous with spectral density,
which implies "magnitude". In the end I wonder whether the notion of phase
even makes sense for a spectrogram ?

Yes, particularly electrical engineering. But there are many other
fields where spectral density is rarely used, and where more "ordinary"
magnitude and phase plots are the norm, as I explained in the previous
paragraphs.

Thanks for dealing with my ignorance. It's true that I have a biased view
on these frequency functions, because I mostly deal with random signals
these days.

In fact I'd like to understand a bit more how phase spectorgram works.
Since the signal must be cut into chunks to make the plot along time, how
is the phase computations "synchronised", that is, how does it use a common
time reference. (because I would imagine that otherwise the phase would
make "jumps" between each window frame). Do you have a pointer for how this
is solved ? (or is this problem just non-existing?).

best,
Pierre