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 FunctionSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Incorrect Result
Status: FixedAssigned to: None
Originator Name: Drew AbbotOpen/Closed: Closed
Release: devOperating System: Any

Add a New Comment (Rich MarkupRich 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 2pi, 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 2pi 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 File(s):
   
   
Comment:
   

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

 

Digest:
   bug dependencies.

Items that depend on this one: None found

 

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

    Do you think this task is very important?
    If so, you can click here to 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
    Fri 08 Aug 2014 03:26:42 AM UTCrik5StatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Thu 07 Aug 2014 05:33:24 PM UTCrik5StatusNone=>Ready For Test
    Tue 29 Jul 2014 06:51:28 AM UTCandy1978Dependencies-=>Depends on bugs #39729
    Mon 28 Jul 2014 05:25:07 AM UTCdrewabbotAttached File-=>Added periodogram.diff, #31783

    Back to the top


    Powered by Savane 3.1-cleanup