matplotlib-devel

[matplotlib-devel] Pull request

[Todd <tod...@gm...>]

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

Re: [matplotlib-devel] Pull request

[Pierre H. <pie...@cr...>]

Hi,

Le 20/10/2013 09:45, Todd a écrit :
> 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

Re: [matplotlib-devel] Pull request

[Todd <tod...@gm...>]

On Mon, Oct 21, 2013 at 3:13 PM, Pierre Haessig <pie...@cr...>wrote:

> Hi,
>
> Le 20/10/2013 09:45, Todd a écrit :
> > 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.

Re: [matplotlib-devel] Pull request

[Todd <tod...@gm...>]

On Sun, Oct 20, 2013 at 9:45 AM, Todd <tod...@gm...> 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] https://github.com/matplotlib/matplotlib/pull/2522
>

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?

Re: [matplotlib-devel] Pull request

[Pierre H. <pie...@cr...>]

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

Re: [matplotlib-devel] Pull request

[Todd <tod...@gm...>]

On Tue, Oct 22, 2013 at 5:41 PM, Pierre Haessig <pie...@cr...>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
>

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.

Re: [matplotlib-devel] Pull request

[Pierre H. <pie...@cr...>]

Hi,

Le 21/10/2013 15:58, Todd a écrit :
> On Mon, Oct 21, 2013 at 3:13 PM, Pierre Haessig
> <pie...@cr... <mailto:pie...@cr...>> 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.
>
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.

Re: [matplotlib-devel] Pull request

[Todd <tod...@gm...>]

On Tue, Oct 22, 2013 at 6:06 PM, Pierre Haessig <pie...@cr...>wrote:

>  Hi,
>
> Le 21/10/2013 15:58, Todd a écrit :
>
>  On Mon, Oct 21, 2013 at 3:13 PM, Pierre Haessig <pie...@cr...
> > 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.
>
>  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.

Re: [matplotlib-devel] Pull request

[Pierre H. <pie...@cr...>]

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

Re: [matplotlib-devel] Pull request

[Todd <tod...@gm...>]

On Fri, Oct 25, 2013 at 2:34 PM, Pierre Haessig <pie...@cr...>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
>

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.

Re: [matplotlib-devel] Pull request

[Pierre H. <pie...@cr...>]

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

Re: [matplotlib-devel] Pull request

[Pierre H. <pie...@cr...>]

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).
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

Re: [matplotlib-devel] Pull request

[Todd <tod...@gm...>]

On Fri, Oct 25, 2013 at 2:57 PM, Pierre Haessig <pie...@cr...>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
>

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.

Re: [matplotlib-devel] Pull request

[Pierre H. <pie...@cr...>]

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.

best,
Pierre

Re: [matplotlib-devel] Pull request

[Todd <tod...@gm...>]

On Fri, Oct 25, 2013 at 3:06 PM, Pierre Haessig <pie...@cr...>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.
>

 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.