bugGNU Octave - Bugs: bug #52758, [octave forge] (signal-1.3.2)...

 
 

bug #52758: [octave forge] (signal-1.3.2) fracshift(x,d) don't shift, if d is an integer

Submitter:  None
Submitted:  Thu 28 Dec 2017 10:23:09 PM UTC
   
 
Category:  Octave Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  juanpi
Originator Name:  Klaus Originator Email:  -email is unavailable-
Open/Closed:  * Closed Release:  * 4.2.1
Operating System:  * Any Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 25 Jan 2018 01:26:44 AM UTC, comment #10: 

finally! I am glad it works as you expected.

The warning can be disabled, I gave and id, so

warning ("off", id)

will put it down. So no issue for somebody running a loop

We need help with several packages. Check control.
And contribute wherever you see you can do it.

Thanks

Juan Pablo Carbajal <juanpi>
Group Member
Mon 22 Jan 2018 11:22:39 PM UTC, comment #9: 

Hello Juan Pablo,
now the function is working correct.
The warning message is a good idea. I hope nobody bothers about the message, when he is calling the function in a loop - 99 times with fractional d and one time with integer d...

I will follow your advice and next time try to became a contributor of myself.

Klaus Braun <kmb721>
Mon 22 Jan 2018 11:24:49 AM UTC, comment #8: 

OK, this should be solved in commit 8ceef8d3b0e1

PLease double check if this is what you expected.

The function ignores the filter issuing a warning.

The returned filter is whatever the user gave as input argument.

If no filter was given, then n filter is returned (i.e. []). could also be NA.

Juan Pablo Carbajal <juanpi>
Group Member
Mon 22 Jan 2018 11:04:47 AM UTC, comment #7: 

Hi,

Ok, I get your point, I was thinkin in shift and not circshift. I will ask you to be more polite in future exchanges, or you will get nothing.

Also you can provide patches instead of just telling around what to do.

Cheers

Juan Pablo Carbajal <juanpi>
Group Member
Sun 21 Jan 2018 05:36:46 PM UTC, comment #6: 

Sorry, but I think your last fix makes no sense.
It is absolutely useless to check if d is integer and then do exactly the same costly calculation, which result is not used. Moreover a complicated h is returned but was not used for the shift. h includes many values about 3e-17 which was not used for the returned y and therfore are inconsistent result.

The sense of the distinction whether d is integer is to have a short and quick return in case of a simple (integer) input.

In case of d = integer you should bypass the complete filter generation and filter calculation, e.g. in the manner I suggested.

In this case, when no filter was applied, I tink it could be ok to return [] as filter.
I have not much set me apart with the theory of fractional shifting and it depends on what you want to do with the returned filter but maybe it could make sense to return 1 as filter in case of integer shift.
To realize this, simply set h = 1 in the if branch of the integer case.
But I am doubtful in the case when d is integer and the caller provides h. Should we return the 'provided' h or schould we return the 'used' 1 - or sould we 'use' the provided h regardless if d is integer and do not the shortcut in this case. I think the last case is the best and provides consistent results.

Klaus Braun <kmb721>
Sun 21 Jan 2018 08:47:24 AM UTC, comment #5: 

Thanks for checking.
The fix solves the reported problem (deosn't work with row vector) but introduced a new bug, namely the saphe of the returned argument.

Fixing this I realized that when the shift was an integer the filter specification wa signored. I fixed both things in the latest commit feea2b59072f

I also added a test. Please check

Juan Pablo Carbajal <juanpi>
Group Member
Sat 20 Jan 2018 11:32:51 PM UTC, comment #4: 

Addendum: I think it's better to change line 38 in:
if (nargin < 2 || nargin > 3)

Klaus Braun <kmb721>
Sat 20 Jan 2018 10:47:48 PM UTC, comment #3: 

Thank you for the quick fix. But -sorry- your commit is not working properly. Now a row vector is returned as a column vector in case of integer shift. There is missing the back transpose y=y.'

Suggested fix:
pack lines 61-79 in an else branch of the if in line 56 and delete the return in line 58.

Klaus Braun <kmb721>
Sat 20 Jan 2018 12:33:50 AM UTC, comment #2: 

I have fixed this issue in the development source tree and improved the function slightly.
Please double check, but I think this is fixed.

commit 131a2629c1a6

Juan Pablo Carbajal <juanpi>
Group Member
Fri 19 Jan 2018 10:55:59 PM UTC, comment #1: 

Is anybody reading bug reports from Anonymous Windows users?
I have now registered and try it again...

fracshift(x,d) does not work if x is a row vector and d is an integer.
If d is an exact integer fracshift uses circshift(x,d) in line 41 of fracshift.m. But circshift expects the dimension to shift as 3rd parameter - if it is omitted it shifts the columns and in case of a row vector it does effectively nothing, see following example:


x = [0 1 0 0 0 0 0 0];  %row vector doesn't work
#x = [0;1;0;0;0;0;0;0];  %column vector works

y = fracshift(x,1);    %integer shift doesn't work
#y = fracshift(x,1.1);  %fractional shift works

subplot(211);
plot(x);
title("original vector x");
subplot(212);
plot(y);
title("shifted vector y");


Suggested bugfix:

circshift(x,d,isrow(x)+1).


Other solution: In line 87-92 there is a "check if input is a row vector" and then the row vector transposed in a column vector. Maybe call circshift(x,d) after this (and before the back transpose).

By the way: bug #47387 (signal-1.2.2) is yet not fixed in signal-1.3.2

Klaus Braun <kmb721>
Thu 28 Dec 2017 10:23:09 PM UTC, original submission:  

If x is a row vector (most used) then fracshift(x,d) is redirected to circshift(x,d), but with a row vector the current code doesn't work. A working code for row vectors is: circshift(x,d,2). I don't know if other input vectors or matrices have to be considered.
Example:

d = randint (1, 1000,[-1,1]);

#dd = circshift(d,3,2);   %works
dd = fracshift(d,3);      %doesn't work
#dd = fracshift(d,3.01);  %works

[acor, lag] = xcorr(d,dd);
plot (lag, abs(acor));
xlim([-5,5]);


Anonymous

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by juanpi (Posted a comment)
  • -email is unavailable- added by kmb721 (Posted a comment)
  • -email is unavailable- added by jwe (Updated the item)
  • -email is unavailable- added by None (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
    2018-01-20 juanpi StatusNone Fixed
        Assigned toNone juanpi
        Open/ClosedOpen Closed
        Operating SystemMicrosoft Windows Any
    2018-01-09 jwe Summarysignal-1.3.2 -&gt; fracshift(x,d) don't shift, if d is an integer [octave forge] (signal-1.3.2) fracshift(x,d) don't shift, if d is an integer

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code