bugGNU Octave - Bugs: bug #64556, unwrap() does not handle NaN and...

 
 

bug #64556: unwrap() does not handle NaN and Inf

Submitter:  Juan <juang>
Submitted:  Tue 15 Aug 2023 11:19:40 AM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  9.1.0 (current default)
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 21 Sep 2023 07:37:14 PM UTC, comment #8: 

No problems reported in a month. Closing as fixed.

Arun Giridhar <arungiridhar>
Group Member
Fri 18 Aug 2023 08:15:27 PM UTC, comment #7: 

ok.  stripped down the important parts of fillmissing to pull the 'next' value for nonfinite elements, and it seems to work. Added a number of tests and input validation, streamlined the function a bit, and pushed to default as
http://hg.savannah.gnu.org/hgweb/octave/rev/a32f36ccc4c1

marking as Ready for Test.

Nicholas Jankowski <nrjank>
Group Member
Wed 16 Aug 2023 12:44:24 PM UTC, comment #6: 

i'll play with it a bit more.  my fillmissing routine was complete but slow.  (uses my repelems implementation for arrays which is not fast).  but i'll see if i can at least get something functional. as long as the path is split off with if(any(!isfinite(x(:)))) it shouldn't hurt general usage.

Nicholas Jankowski <nrjank>
Group Member
Wed 16 Aug 2023 03:38:39 AM UTC, comment #5: 

I had a hunch it wouldn't be that simple. 

I also wondered about the performance of the algorithm.  The indexing part looked slow and maybe a permute/ipermute would be faster.

I agree that since this change is going on the development branch we might as well overhaul the whole thing.

Rik <rik5>
Group administrator
Wed 16 Aug 2023 02:56:44 AM UTC, comment #4: 

as a side note since we're messing with this function, the original part that gets you d using repmat seems rather slow.  replacing with something like:


    sz(dim) = 1;
    zero_padding = zeros (sz);
    d = cat (dim, zero_padding, -diff (x, 1, dim));


tested about 6-10x faster

Nicholas Jankowski <nrjank>
Group Member
Wed 16 Aug 2023 02:53:31 AM UTC, comment #3: 

that patch doesn't quite do it.  it works for the test values because none of the numbers adjacent to the non-finite values is > 180deg away.  if you adjust the number before the NaN:


x=[10   100  -190   NaN  -100   Inf   -30];


Matlab gives:

>> (180/pi)*unwrap((pi/180)*x)

ans =

   10.0000  100.0000  170.0000       NaN  260.0000       Inf  330.0000


your patch for octave gives:


>> (180/pi)*unwrap((pi/180)*x)
ans =>

    10   100   170   NaN  -100   Inf   -30


it picks up another jump from the from the 190-to-0 step.


i was playing with this a bit.  for vectors a simple shortcut is something like:


    if (isvector (x))
      ## Simlpified path for vector inputs.

      retval = x;
      xfin_idx = isfinite (x);
      xfin = x(xfin_idx);
      d = [0, -diff(xfin, 1, dim)];
      p = round (abs (d)./rng) .* rng .* ...
                      (((d > tol) > 0) - ((d < -tol) > 0));
      r = cumsum (p, dim);
      retval(xfin_idx) = xfin + r;
    else

for the rest to work on n-D arrays, instead of zero you could sub in the previous non-nonfinite value.  that would guarantee no shift and that element would have no effect on the calculation. then you could restore the nonfinite vals just like your patch did.

grabbing the previous value is something i wrote up for statistics `fillmissing` function. was looking at permuting operation to dim1 then borrowing a simpler version of that.  non-finites at the beginning and two-in-a-row make it nontrivial.

unless there's an easier way i'm missing.
...


Nicholas Jankowski <nrjank>
Group Member
Wed 16 Aug 2023 01:18:36 AM UTC, comment #2: 

I confirm as well.

Matlab does seem to just skip over the non-finite values (-Inf, +Inf, NaN).  I tried this example


w = rad2deg (unwrap (deg2rad ([-200, Inf, 500])))


and it was equivalent to


w = rad2deg (unwrap (deg2rad ([-200, 500])))


which implies that the wrapping state is not zeroed out by a non-finite value but simply retains the state from the previous valid value.

I think a lot of Octave functions haven't been tested rigorously for edge cases, and this is just one of those instances.

I took a brief look at unwrap.m as well and it looks like you need to create an index with "isfinite (x)" and then perform the calculations only on those elements and then restore the non-finite values at the end of the function.  See attached changeset  which I think works and has a BIST test for this bug.

(file #55045)

Rik <rik5>
Group administrator
Tue 15 Aug 2023 02:22:29 PM UTC, comment #1: 

Confirmed on latest default.

It seems there are quite a few places popping up with Inf/NaN handling differences. wondering if this is new behavior or if we've just missed it for some time. They did introduce a NANFLAG in many functions that allows processing skipping over NaNs, wondering if trickle down from that is what's popping up here.

it looks like in the presence of inf and nan, matlab performs the unwrap as if they weren't present, leaving the inf and nan in place in the answer.

looking at how the function builds up the answer for vectors, this should be fairly easy to notch out the non-numbers. but will need to double check with 2D and nD arrays and with specified dimensions as notching may create an unequal number of elements in the operating dimension. may have to introduce some masking instead.

Nicholas Jankowski <nrjank>
Group Member
Tue 15 Aug 2023 11:19:40 AM UTC, original submission:  

Octave's unwrap() function does not handle NaNs and Infs as the Matlab equivalent. Instead of skipping them, all output following a single NaN input is NaN etc.

See the following example

Octave:

>> (180/pi)*unwrap((pi/180)*[10,100, -175, NaN, -100, Inf, -30])
ans =

    10   100   185   NaN   NaN   NaN   NaN

Matlab:

>> (180/pi)*unwrap((pi/180)*[10,100, -175, NaN, -100, Inf, -30])

ans =

    10   100   185   NaN   260   Inf   330


Juan <juang>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #55045:  bug64556.cset added by rik5 (2KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by arungiridhar (Posted a comment)
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by juang (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 project members can vote.

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-09-21 arungiridhar StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2023-08-18 nrjank StatusPatch Submitted Ready For Test
    2023-08-16 rik5 Attached File- Added bug64556.cset, #55045
        CategoryLibraries Octave Function
        StatusConfirmed Patch Submitted
        Planned ReleaseNone 9.1.0 (current default)
    2023-08-15 nrjank StatusNone Confirmed
        Release8.2.0 dev
        Operating SystemMicrosoft Windows Any

    Back to the top

    Powered by Savane 3.12