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

 
 

bug #42859: bugs in periodogram.m

Submitter:  Drew Abbot <drewabbot>
Submitted:  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
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

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>
Group administrator
Thu 07 Aug 2014 11:56:32 PM UTC, comment #3: 

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


 pxx = periodogram (1:8, [], 4)


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)


 pxx = periodogram (1:8, [], 4, 'onesided')


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


 if (nargin<4)
   Pxx /= 2*pi;
 elseif (! isempty (fs))
   Pxx /= fs;
 endif


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


pxx = periodogram (1:8, [], 4)


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


x = sum (reshape (x, nfft, []), 2)


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>
Group 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:


  Pxx = Pxx(1:nfft/2+1) + [0; Pxx(end:-1:(nfft/2+2)); 0];


To witness the bug, simply attempt:


  pxx = periodogram( sin(1:10) )


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:


  pxx = periodogram( sin(1:5)', [], 5 )


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

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    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.13-f8d8.
    Corresponding source code