bugGNU Octave - Bugs: bug #42859, bugs in periodogram.m

 
 

bug #42859: bugs in periodogram.m

Submitted by:  Drew Abbot <drewabbot>
Submitted on:  Mon 28 Jul 2014 05:25:07 AM UTC  
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Drew Abbot Open/Closed:  Closed
Release:  dev Operating System:  Any

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

Fri 08 Aug 2014 03:38:26 AM UTC, comment #5:

Sweet! If I get time, I'll suggest possible tests to add, and/or other possible additions like 'centered' support.

Drew Abbot <drewabbot>
Fri 08 Aug 2014 03:26:42 AM UTC, comment #4:

I checked in another changeset which better handles the input parsing and deciding whether to us Fs or normalized frequencies (http://hg.savannah.gnu.org/hgweb/octave/rev/956fc864c39f). Things seem okay for now, although in the future it might be nice to support the other options, like 'centered', that Matlab does.

Rik <rik5>
Project Administrator
Thu 07 Aug 2014 11:56:32 PM UTC, comment #3:

As far as I can tell, as of MATLAB 2013a:

yields [ 25.7831 ; 1.2732 ; 0.3183 ], and Octave yields the same result.

However, in MATLAB, the following also yields that same result (as it should)

but in Octave, the result is [ 162 ; 8 ; 2 ], which is the same result as before but scaled by (2*pi). This is related to both the code

and the fact that fs is initialized to []. To get around this, fs could be initialized to 2*pi, or to be even more complete, multiple MATLAB compatibility modes could be supported (as is done in the Octave signal package's pwelch() method), with default fs values of 1.0 or 2*pi accordingly.

Drew Abbot <drewabbot>
Thu 07 Aug 2014 05:33:24 PM UTC, comment #2:

periodogram was badly in need of updating. I made your changes as well as overhauling the documentation, simplifying the input argument parsing, and adding input validation tests in this changeset (http://hg.savannah.gnu.org/hgweb/octave/rev/8ac4ab4ae5f4).

If someone has access to the Matlab signal toolbox could they try

According to what I can decipher, Octave will get this wrong by a factor of 2 because it doesn't adjust 'n', the number of rows in X, after it downsized X to match NFFT with this statement

The function also needs some functional tests badly. This was a major rewrite and I had to guess that everything worked out because there are no built-in %!tests.

Rik <rik5>
Project Administrator
Mon 28 Jul 2014 05:53:57 AM UTC, comment #1:

I just realized the first bug discussed here is a repeat of bug #39729, so I posted the same .diff there..

Drew Abbot <drewabbot>
Mon 28 Jul 2014 05:25:07 AM UTC, original submission:

There are two bugs in the current periodogram() implementation, and here's a new version which fixes both. The first bug is that, when using one-sided spectra (the default behavior), only column vectors are supported for the input signal, x. Sending in row vectors causes a "vertical dimensions mismatch" error due to this one-sided conversion code:

To witness the bug, simply attempt:

The second also involves that one-sided conversion code. Namely, it is incorrect when nfft is odd, since it yields 1/2 the correct value at the upper-most (non-Nyquist) frequency.

To see this, note that the following returns ~0.0171 for the third component, when it should return ~0.0342:

The proposed fixes address both issues (using similar one-sided conversion code to that found in the current pwelch() implementation of the signal package), and also forces that the window (if non-empty) is a vector of the same length as the signal (which is the current MATLAB behavior).

Drew Abbot <drewabbot>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #31783:  periodogram.diff added by drewabbot (1KiB - application/octet-stream)

 

Digest:
   bug dependencies.

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by drewabbot (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2014-08-08 rik5 StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2014-08-07 rik5 StatusNone => Ready For Test
    2014-07-29 andy1978 Dependencies- => Depends on bugs #39729
    2014-07-28 drewabbot Attached File- => Added periodogram.diff, #31783

    Back to the top


    Powered by Savane 3.3