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

Submitted by:  None
Submitted on:  Thu 28 Dec 2017 10:23:09 PM UTC  
 
Category: Octave Forge PackageSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Incorrect Result
Status: FixedAssigned to: Juan Pablo Carbajal <juanpi>
Originator Name: KlausOriginator Email: -unavailable-
Open/Closed: ClosedRelease: 4.2.1
Operating 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)

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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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:

Suggested bugfix:

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:

Anonymous

 

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

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by juanpi (Posted a comment)
  • -unavailable- added by kmb721 (Posted a comment)
  • -unavailable- added by jwe (Updated the item)
  • -unavailable- added by None (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
    Sat 20 Jan 2018 12:33:50 AM UTCjuanpiStatusNone=>Fixed
      Assigned toNone=>juanpi
      Open/ClosedOpen=>Closed
      Operating SystemMicrosoft Windows=>Any
    Tue 09 Jan 2018 08:40:52 PM UTCjweSummarysignal-1.3.2 -> 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.1-cleanup1