bugGNU Octave - Bugs: bug #33625, filter with dimension and empty SI...

 
 

bug #33625: filter with dimension and empty SI does return error

Submitter:  Sebastien <se6>
Submitted:  Thu 23 Jun 2011 06:50:59 AM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Seb Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 22 Nov 2011 11:32:20 PM UTC, comment #11: 

Sure. This has been quite instructive for me. Sorry for taking time from your day.
Let me point out that I added a printf("debug note...") statement to the filter.cc file before building so that I could show you what I mentioned before.

Here it goes:

1) Octave 3.4.3 stable, patched with the patch in this thread (the error comes out, and please observe si_dims(0)).

octave:1> version
ans = 3.4.3
octave:2> y = filter(ones(1,6)/6,1,randn(1000,1),zeros(5,1));
debug note: si_dims(0) = 1
error: filter: first dimension of SI must be of length max (length (a), length (b)) - 1
octave:2> y = filter(ones(1,6)/6,1,randn(1000,1),zeros(1,5));
debug note: si_dims(0) = 1
error: filter: first dimension of SI must be of length max (length (a), length (b)) - 1

2) Octave 3.4.3 stable, with the patch and the extra fix I had mentioned (charm):

octave:1> version
ans = 3.4.3
octave:2> y = filter(ones(1,6)/6,1,randn(1000,1),zeros(5,1));
debug note: si_dims(0) = 1
octave:3> y = filter(ones(1,6)/6,1,randn(1000,1),zeros(1,5));
debug note: si_dims(0) = 1

3) Octave 3.5.0+ from Mercurial, built a few minutes ago (note the change in si_dims(0), that's why the code in comment 8 worked for you):

octave:1> version
ans = 3.5.0+
octave:2> y = filter(ones(1,6)/6,1,randn(1000,1),zeros(5,1));
debug note: si_dims(0) = 5
octave:3> y = filter(ones(1,6)/6,1,randn(1000,1),zeros(1,5));
debug note: si_dims(0) = 5

So yes, the new version will have a fixed filter() =). But there are other things involved, such as the rare behavior over SI (transposed, not transposed, ???).

I guess it is settled, right?

Thanks again

Oscar Yanez <oscarys>
Tue 22 Nov 2011 09:22:26 PM UTC, comment #10: 

Can you provide a simple example in Octave code that demonstrates the problem?  The code in comment #8 runs fine for me with the patched development version of Octave.

Rik <rik5>
Group administrator
Tue 22 Nov 2011 08:05:15 PM UTC, comment #9: 

Thanks Rik and Jordi for your prompt replies. Let me tell you though that the patch didn't work for me (fails in the same place, the very first test) and that was the reason for the tests and changes I comment in my previous post.

Thanks,
Oscar

Oscar Yanez <oscarys>
Tue 22 Nov 2011 07:59:39 PM UTC, comment #8: 

Hello again,

I've been looking through this for the morning and I have a bunch of information to share. First, let me confirm that the file filter.cc that comes with the current 3.4.3 source download does not have the patch described in this thread. So whoever builds from this source will encounter the filter() problem.

Second, I added some debugging code to filter.cc and noticed that it doesn't matter if the SI parameter in the function call is a row or a column vector (it is expected to be a column vector), it always shows up in filter.cc as a row vector (!!), and therefore si_dims(0) in line 93 is ALWAYS one. That makes the test fail right there, because the state vector should be of length ab_len-1, which is the value of si_dims(1) when the function is called with the correct arguments. I changed that single index and the function works as expected. Maybe I am overlooking errors further down in the code, but my tests have run well.

Before transcribing some of my test code, may I ask why is it that SI ends up being a row vector no matter what? I have little knowledge of the calling process in octave, but it is clear that SI is changed.

The test code that follows filters a random signal in two different ways: first, it filters the entire vector with zero initial state values; second, it filters the vector in halfs, passing the state vectors from call to call. Differences in the result should be negligible or zero and so they are:

octave:3> x = randn(1000,1);
octave:4> b = [0.2 0.4 0.2]; a = 1; si = zeros(2,1);
octave:5> y = filter(b,a,x,si);
octave:6> [y1,sf] = filter(b,a,x(1:500),si);
octave:7> y2 = filter(b,a,x(501:1000),sf);
octave:8> max(abs((y-[y1;y2])))
ans = 0

To corroborate the fact that SI is always made into a row vector, the call:

octave:12> y = filter(b,a,x,si);

works the same (with no error messages) as

octave:13> y = filter(b,a,x,si');

after the change I propose. So, what do you think ?

Best,
Oscar

Oscar Yanez <oscarys>
Tue 22 Nov 2011 05:34:30 PM UTC, comment #7: 

The bug was fixed on the development branch of Octave, not the stable 3.4.X release series.  If you need to see the fix you can build from Mercurial sources.  See the download page (http://www.gnu.org/software/octave/download.html) for getting the sources.

Alternatively, the 3.6.0 release is scheduled for a December 2011 release and it will have the fixed code.

Rik <rik5>
Group administrator
Tue 22 Nov 2011 05:34:20 PM UTC, comment #6: 

Oscar, the patch was on the development branch, which should soon become stable within a month or so. The fix hasn't been released yet.

If you're feeling adventurous and urgently need this patch, you will have to build Octave yourself, either from the stable source and importing this patch, or building the current development sources.

Jordi GutiƩrrez Hermoso <jordigh>
Group Member
Tue 22 Nov 2011 05:26:42 PM UTC, comment #5: 

Sorry to disturb on a closed topic, but I beleive the problem with the filter() function remains as of today:

(1) installed Octave from the Fedora 16 rpm's ... filter() fails
(2) removed that installation, compiled a fresh Octave 3.4.3 from sources downloaded today and executed ./run-octave after build.... filter() fails again.

Is it that the patch has not been integrated to current sources? I'm unfamiliar with the patching process but will give it a try and report.

All the best,
Oscar

Oscar Yanez <oscarys>
Fri 24 Jun 2011 09:31:37 PM UTC, comment #4: 
Rik <rik5>
Group administrator
Fri 24 Jun 2011 08:03:02 PM UTC, comment #3: 

I have tried the patch. It seems to work fine indeed, and is way better than my removing of the test!

Thanks.

Seb

Sebastien <se6>
Fri 24 Jun 2011 07:28:42 PM UTC, comment #2: 

Bug confirmed.  I think the best solution is to fix the input validation, rather than to remove it. 

Please try the patch I am attaching to this bug report.  If it works reliably I will commit it to the repository.

Rik <rik5>
Group administrator
Thu 23 Jun 2011 07:19:02 AM UTC, comment #1: 

I have tried that change, and this seems to work:
It now returns the correct result. It passes all the tests and the examples in bug #32741

Regards,

Seb

--- filter.cc.original 2011-06-22 21:12:35.375952955 +0200
+++ filter.cc 2011-06-23 08:57:30.000000000 +0200
@@ -98,12 +98,6 @@
       return y;
     }
 
-  if (si_dims.length () != x_dims.length ())
-    {
-      error ("filter: dimensionality of SI and X must agree");
-      return y;
-    }
-
   octave_idx_type si_dim = 0;
   for (octave_idx_type i = 0; i < x_dims.length (); i++)
     {
@@ -113,11 +107,6 @@
       if (x_dims(i) == 1)
         continue;
 
-      if (si_dims(++si_dim) != x_dims(i))
-        {
-          error ("filter: dimensionality of SI and X must agree");
-          return y;
-        }
     }

Sebastien <se6>
Thu 23 Jun 2011 06:50:59 AM UTC, original submission:  

Hi,

a = ones(2,1,3,4);
a(1,1,:,:) = [1 2 3 4; 5 6 7 8; 9 10 11 12];
y = filter([1 1 1],1,a,[],3);
y(:)'

% That generates an error:
% error: filter: dimensionality of SI and X must agree
%
% It should return:
%
% ans =
%  Columns 1 through 17
%  1     1     6     2    15     3     2     1     8     2   
% 18     3     3     1    10     2    21
%  Columns 18 through 24
%  3     4     1    12     2    24     3
%

That affect the latest dev but also 3.2, 3.4. (Don't know about earlier versions)

Regards,

Seb

Sebastien <se6>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #23555:  filter.patch added by rik5 (2KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jordigh (Posted a comment)
  • -email is unavailable- added by oscarys (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by se6 (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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-06-24 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2011-06-24 rik5 Attached File- Added filter.patch, #23555
    2011-06-24 rik5 StatusNone Patch Submitted

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code