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

 
 

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

Submitted by:  Sebastien <se6>
Submitted on:  Thu 23 Jun 2011 06:50:59 AM UTC  
 
Category: InterpreterSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Incorrect Result
Status: FixedAssigned to: None
Originator Name: SebOpen/Closed: Closed
Release: devOperating System: Any

Add a New Comment(Rich Markup)
   

You are not logged in

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

 

(Jump to the original submission 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>
Project 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>
Project 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>
Project Administrator
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:

Fixed in this changeset (http://hg.savannah.gnu.org/hgweb/octave/rev/ec6c52496485)

Rik <rik5>
Project 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>
Project 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 File(s):
   
   
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
  • -unavailable- added by jordigh (Posted a comment)
  • -unavailable- added by oscarys (Posted a comment)
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by se6 (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 4 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 24 Jun 2011 09:31:37 PM UTCrik5StatusPatch Submitted=>Fixed
      Open/ClosedOpen=>Closed
    Fri 24 Jun 2011 07:29:01 PM UTCrik5Attached File-=>Added filter.patch, #23555
    Fri 24 Jun 2011 07:28:42 PM UTCrik5StatusNone=>Patch Submitted

    Back to the top


    Powered by Savane 3.1-cleanup1