Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add additional spectrum-related plots and improve underlying structure #2522

Merged
merged 9 commits into from Oct 24, 2013
Merged

Add additional spectrum-related plots and improve underlying structure #2522

merged 9 commits into from Oct 24, 2013

Conversation

toddrjen
Copy link
Contributor

This set of patches has two main purposes. First, to add additional frequency spectrum plot types. This includes magnitude, phase, and angle spectrums. It also adds magnitude, phase, and angle spectrums to the existing specgram function.

However, to accomplish this, improve the performance of the existing plots, and make testing easier, I have substantially refactored the underlying functions. Both the plot types, the mlab functions they use, and the underlying functions those rely have all had their tests substantially increased. They should now all have 100% or close to 100% coverage.

I have also made the documentation of both those functions and mlab as a whole bother more consistent and more complete.

I have also expanded the capabilities of a number of the other mlab functions, primarily the detrend and windowing functions. These changes were made to help the spectrum-related functions, but can also be used independently of those functions.

@toddrjen
Copy link
Contributor Author

The two stride functions I added to mlab are based on code snippets from strackoverflow, I am not sure what the policy on that is.

@tacaswell
Copy link
Member

SO content is CC-SA http://stackoverflow.com/help/licensing

What is the license for the content I post?

As noted in the Stack Exchange Terms of Service and in the footer of every page, all user contributions are licensed under Creative Commons Attribution-Share Alike. Proper attribution is required if you republish any Stack Exchange content.

Please read the terms of service and the full legal text of the license carefully for more details on how your content can be used.

Which looks like it is BSD compatible, but IANAL

@toddrjen
Copy link
Contributor Author

This comment suggests that attribution is only necessary if you post whole pages, not small code snippets:
http://meta.stackoverflow.com/a/139701

However, I have added references to the answers anyway just to be safe. It also helps future people know where it all came from if they want to understand what is going on.

@toddrjen
Copy link
Contributor Author

I have rebased this after the recent merges, so it should apply cleanly again.

I think this set of plot types is important. Being able to plot standard fourier transforms is a very common operation in signal processing and would also be very useful for educational environments (such as signals courses). I have had a number of colleagues complain that Matlab does not have such plots built in.

@Tillsten
Copy link
Contributor

Python has an inbuild "callable" function, which does exactly the same as is_function_like.

@@ -5786,7 +6079,8 @@ def cohere(self, x, y, NFFT=256, Fs=2, Fc=0, detrend=mlab.detrend_none,
def specgram(self, x, NFFT=256, Fs=2, Fc=0, detrend=mlab.detrend_none,
window=mlab.window_hanning, noverlap=128,
cmap=None, xextent=None, pad_to=None, sides='default',
scale_by_freq=None, **kwargs):
scale_by_freq=None, mode='default', scale='default',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many cases in the code where None is used as the default value.

This has the advantage that you can then wrap the function with another function with takes optional kwargs without ending up with the default value stored multiple places and with out worrying about all functions not having **kwarg in their signatures.

This comment should be applied to many of your function signatures.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defaults have been changed to None wherever possible.

@toddrjen
Copy link
Contributor Author

Implemented all suggestions so far:

Used None for argument defaults whenever possible.

Removed redundant test.

Switched to using builtin callable function and removed is_function_like function.

The changes have been squashed into the existing commits.

@toddrjen
Copy link
Contributor Author

Rebased on latest commit.

@ewmoore
Copy link

ewmoore commented Oct 21, 2013

Does this fix #1561?

@toddrjen
Copy link
Contributor Author

Yes, #1561 was caused by having an odd number of data points in NFFT, which is mathematically incorrect and can lead to unpredictable behavior.

This is no longer allowed. The documentation said that NFFT had to be even, but it wasn't enforced. Now trying to do this will raise an exception.

@ewmoore
Copy link

ewmoore commented Oct 22, 2013

I suppose that is a fix. Not sure what you mean about mathematically invalid and unpredictable. The DFT is defined just fine for N odd, and numpy's fttpack based fft() function supports fast transforms for N a composite of 2, 3, 5 and can do a transform for any N.

@pierre-haessig
Copy link
Contributor

About NFFT argument, I agree that there is no need to force it to be even. http://www.mathworks.fr/fr/help/signal/ref/periodogram.html?searchHighlight=periodogram#inputarg_nfft


# if NFFT is set to None use the whole signal
if NFFT is None:
NFFT = 256
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get the if NFFT is None: NFFT = 256. Why not keep the default value 256 in the function signature then ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is precedent elsewhere in the library of using None as the default
value.

You frequently want to wrap matplotlib functions with your own (that do
your data extraction and clean up, set up multi-panel figures, or what
ever). By using None as the default you can have optional kwargs to your
own functions (which also deault to None) and get sensible behavior
without having to transcribe the defaults into your code.

On Tue, Oct 22, 2013 at 11:28 AM, Pierre Haessig
notifications@github.comwrote:

In lib/matplotlib/mlab.py:

  •    #implement the core of psd(), csd(), and spectrogram() without doing
    
  •    #extra calculations.  We return the unaveraged Pxy, freqs, and t.
    
  •    same_data = y is x
    
  • if Fs is None:
  •    Fs = 2
    
  • if noverlap is None:
  •    noverlap = 0
    
  • if detrend_func is None:
  •    detrend_func = detrend_none
    
  • if window is None:
  •    window = window_hanning
    
  • if NFFT is set to None use the whole signal

  • if NFFT is None:
  •    NFFT = 256
    

I don't get the if NFFT is None: NFFT = 256. Why not keep the default
value 256 in the function signature then ?


Reply to this email directly or view it on GitHubhttps://github.com//pull/2522/files#r7131669
.

Thomas Caswell
tcaswell@gmail.com

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also helps with pyplot. boilerplate.py copies over the default argument. If these are None, you can change the defaults without having boilerplate.py changing anything in pyplot, which keeps the git history cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the meaning of default value varies between cases. For something like detrend=None, it's pretty clear that it means "no detrending". But for NFFT=None, how is one suppose to guess that it means in NFFT=256 deeply buried in the code ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is in the docstring.

@toddrjen
Copy link
Contributor Author

I have rewritten a big chunk of code to allow odd NFFT and pad_to values to be used. I have also implemented tests for odd NFFT, pad_to, and len(x). As a result, odd NFFT values no longer raise exceptions. So #1561 should be properly fixed now.

@toddrjen
Copy link
Contributor Author

I have also implemented support for the "return_line" argument in psd and csd, which allows the plots to optionally return the line object they create (by default they don't, which is the current behavior).

I think ideally this would be the only behavior, but for backwards-compatibility reasons this should probably be kept for a little while. Perhaps down the road the current behavior can be deprecated, then later the default changed, then later still have the argument removed entirely.

The unit tests also now test this argument.

n = len(x)
x = np.resize(x, (NFFT,))
x[n:] = 0

if not same_data and len(y)<NFFT:
if not same_data and len(y) < NFFT:
n = len(y)
y = np.resize(y, (NFFT,))
y[n:] = 0
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do this? You can get the same effect as zero paddding by telling np.fft.fft to do a longer transform. More importantly, you absolutely don't want to use this zero padded size for the window, which ends up happening below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zero-padding the FFT is controlled by the pad_to argument. The NFFT argument controls the size of the sliding window used for psd, csd, and specgram. This line makes sure that there is at least one window. It is never invoked with magnitude_spectrum, phase_spectrum, or angle_spectrum, or complex_spectrum, for these NFFT == len(x).

@toddrjen
Copy link
Contributor Author

If no one else has any other comments, would it be possible to go ahead and merge this? It is blocking some other stuff I want to do.

mdboom added a commit that referenced this pull request Oct 24, 2013
Add additional spectrum-related plots and improve underlying structure
@mdboom mdboom merged commit d5f9876 into matplotlib:master Oct 24, 2013
@toddrjen
Copy link
Contributor Author

Thanks!

@@ -76,6 +76,28 @@ original location:
- `xycoords` -> set the units of the point location
- `set_position()` -> `Annotation` only set location of annotation

* NFFT being even is now enforced in `matplotlib.mlab.specgram`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this line be removed from the changes ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I will try to submit a fix for this later tonight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants