bugGNU Octave - Bugs: bug #64395, corr cannot handle row vectors

 
 

bug #64395: corr cannot handle row vectors

Submitter:  Dmitri A. Sergatskov <dasergatskov>
Submitted:  Sat 08 Jul 2023 06:50:29 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  9.1.0 Planned Release:  9.1.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 17 Oct 2023 03:37:18 AM UTC, comment #17: 

that's a matlab compatibility change made over on bug #64555, that actually fixed this bug's incomplete compatibility fix for row vectors, which was a follow on to a compatibility overhaul for corr and cov from bug #50583.

Matlab 2023b

>> x = [6 7 7 9 10 12 13 14 15 17]

x =

     6     7     7     9    10    12    13    14    15    17

>> y = [19 22 27 25 30 28 30 29 25 32]

y =

    19    22    27    25    30    28    30    29    25    32

>> r = corr (x, y)

r =

   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN
   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN
   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN
   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN
   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN
   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN
   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN
   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN
   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN
   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN   NaN


we caught and fixe any core octave impacts of the corr and cov compatibility change, but expected it was going to impact a number of package functions

Nicholas Jankowski <nrjank>
Group Member
Tue 17 Oct 2023 12:40:47 AM UTC, comment #16: 

While testing bug #64778, I noticed failures with gevfit and correlation_test. I found this report after a search.

When digging into why I was getting an XERBLA / DGECON error for a correlation coefficient calculation, I found that one of the statements in correlation_test gives a big matrix full of Nans, and it was because they were row vectors:


x = [6 7 7 9 10 12 13 14 15 17]
y = [19 22 27 25 30 28 30 29 25 32]

[h, pval, stats] = correlation_test (x, y)  # fails with BLAS errors

r = corr (x, y)   # returns big matrix of Nan


What is r supposed to be in that line? A single scalar correlation coefficient? What does Matlab do for that line?

Should this bug be reopened?

Arun Giridhar <arungiridhar>
Group Member
Mon 17 Jul 2023 04:49:41 PM UTC, comment #15: 

That change seems to broke kendall().


***** test
 x = [1:2:10];
 y = [100:10:149];
 assert (kendall (x,y), 1, 5*eps);
 assert (kendall (x,fliplr (y)), -1, 5*eps);
***** assert (kendall (logical (1)), 1)
!!!!! test failed
ASSERT errors for:  assert (kendall (logical (1)),1)

  Location  |  Observed  |  Expected  |  Reason
     ()          NaN           1         'NaN' mismatch


I filled a separate bug report for this.
<https://savannah.gnu.org/bugs/?64435>

Dmitri.
--


Dmitri A. Sergatskov <dasergatskov>
Mon 17 Jul 2023 03:00:23 PM UTC, comment #14: 

I checked in a change so that Octave produces NaN for scalar input (http://hg.savannah.gnu.org/hgweb/octave/rev/1e760618ae06).  This is Matlab-compatible and also solves the issue that scalars were not subject to input validation.

I'm going to call this bug Fixed and close the report.

Rik <rik5>
Group administrator
Tue 11 Jul 2023 04:33:20 PM UTC, comment #13: 

I checked in an update for the message strings used in BIST testing.  "test corr" now passes.

Rik <rik5>
Group administrator
Tue 11 Jul 2023 02:53:30 AM UTC, comment #12: 

Rik -- I think you last change messed up some corr() BISTs.


Author        Rik <rik@octave.org>
Date        July 10, 2023 8:01 PM (3 hours ago)
Repository        https://hg.savannah.gnu.org/hgweb/octave
Branch        default
Revision        e4e7bc93f5f718b92611be957c73a2539f40c3f7



***** error <inputs must be the same size> corr (ones (2,2), ones (2,2,2))
!!!!! error failed.
Expected <inputs must be the same size>, but got <corr: X and Y must be the same size>
***** error <inputs must be the same size> corr ([1,2,3], [1,2,3]')
!!!!! error failed.
Expected <inputs must be the same size>, but got <corr: X and Y must be the same size>
***** error <inputs must be the same size> corr ([1,2,3]', [1,2,3])
!!!!! error failed.
Expected <inputs must be the same size>, but got <corr: X and Y must be the same size>



Dmitri.

Dmitri A. Sergatskov <dasergatskov>
Tue 11 Jul 2023 12:34:39 AM UTC, comment #11: 

Hmmm, when rewriting cov and corr I was trusting cov to do all the input validation so we didn’t pay that penalty twice with corr. Need to review both I guess for a cariety of inputs.  (I assume cov also shouldn’t accept something like a struct() input). Traveling and less access to Matlab to build a target BiST set at the moment, but will give it a look next chance i get if no one else puts a list together first

Nicholas Jankowski <nrjank>
Group Member
Tue 11 Jul 2023 12:04:16 AM UTC, comment #10: 

I notice that corr() for a scalar always returns 1.  This is different from Matlab which returns NaN.  Simple test case:


corr (5)


Also, shouldn't non-numeric input throw an error?  For the scalar special case there is no such checking.


corr (struct ())
ans = 1


Matlab throws an obtuse error, but still an error, for the above code.

Rik <rik5>
Group administrator
Mon 10 Jul 2023 12:07:32 AM UTC, comment #9: 

some of that has already been captured over at bug #64324.  probably should post these details over there, since they came from the same set of compatibility updates to cov and corr.

Nicholas Jankowski <nrjank>
Group Member
Sun 09 Jul 2023 11:47:23 PM UTC, comment #8: 

The failing tests are:


Failure Summary:

  ..re/octave/api-v58/packages/statistics-1.6.0/dist_fit/gevfit.m  pass    4/6
                                                                   FAIL    2

 ..e/octave/api-v58/packages/statistics-1.6.0/correlation_test.m  pass   18/19
                                                                   FAIL    1

 ../share/octave/api-v58/packages/statistics-1.6.0/stepwisefit.m  pass    0/1
                                                                   FAIL    1


But there is something else going on with statistics. If I run
"pkg test statistics" twixe, on the second run I get some errors
in boxplot and then it hangs in dendrogram. I will post on discourse about it.

Dmitri.
--



Dmitri A. Sergatskov <dasergatskov>
Sun 09 Jul 2023 11:29:52 PM UTC, comment #7: 

Now I have few FAILs in statistics...



Dmitri
--

Dmitri A. Sergatskov <dasergatskov>
Sun 09 Jul 2023 10:50:26 PM UTC, comment #6: 

ok.  cov was behaving correctly so the fix was in corr.  as shown below row vector inputs are still treated as single observation tests resulting in 0 std and an output of nan(num_columns).  added a shortcut path for that and BISTs to both corr and cov.  passes all tests, and current default passes a make check so it doesn't seem to break anything else in core.

pushed to default as

http://hg.savannah.gnu.org/hgweb/octave/rev/37e184a83cf4.

marking as ready for test.

Nicholas Jankowski <nrjank>
Group Member
Sun 09 Jul 2023 05:24:48 PM UTC, comment #5: 

The easy fix is to convert inputs to column vectors e.g.: "corr(a(:),b(:))"

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Sun 09 Jul 2023 05:21:44 PM UTC, comment #4: 

Same thing happened with a couple core functions as noted in bug #50583.  I think a couple of them retained the old behavior, so you might be able to look there to see if it was an easy fix.

Nicholas Jankowski <nrjank>
Group Member
Sun 09 Jul 2023 03:06:47 PM UTC, comment #3: 

OK thanks. That change broke filtfilt BIST. I will make a separate bug report.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Sun 09 Jul 2023 02:53:53 PM UTC, comment #2: 

note that while the error is obviously a problem, the 'good' output noted in comment #1 reflects the old behavior of cov/corr.  the change referenced below updated those functions to be matlab compatible, and the correct (just checked with v2023a) output should be:


>> a = [1,2,3]
a =
     1     2     3
>> b = a
b =
     1     2     3
>> corr(a,b)
ans =
   NaN   NaN   NaN
   NaN   NaN   NaN
   NaN   NaN   NaN
>> a = [1,2,3]'
a =
     1
     2
     3
>> b = a
b =
     1
     2
     3
>> corr(a,b)
ans =
    1.0000


will add explicit BISTS, see what needs to be done in cov where i suspect the problem lives, and see if it's a quick fix.

Nicholas Jankowski <nrjank>
Group Member
Sat 08 Jul 2023 08:38:43 PM UTC, comment #1: 

This is due to:

changeset:   31918:e67b7b85670b
user:        Nicholas R. Jankowski <jankowski.nicholas@gmail.com>
date:        Wed Mar 22 13:33:46 2023 -0400
summary:     cov.m: Overhaul function for matlab compatibility (bug #50583).


Good:

octave:1> a=[1,2,3]; b = a; corr(a,b)
ans = 1
octave:2> version -hgid
ans = 655e757c7522
octave:3>


Bad:
octave:1> a=[1,2,3]; b = a; corr(a,b)
error: c(3,_): out of bound 1 (dimensions are 1x1)
error: called from
    corr at line 86 column 7
octave:2> version -hgid
ans = e67b7b85670b
octave:3>
-verbatim-

Not sure if the fix should happen in corr.m or in cov.m
(https://savannah.gnu.org/bugs/?50583)

Dmitri.
--



Dmitri A. Sergatskov <dasergatskov>
Sat 08 Jul 2023 06:50:29 PM UTC, original submission:  


octave:1> a=[1,2,3]
a =

   1   2   3

octave:2> b = a
b =

   1   2   3

octave:3> corr(a,b)
error: c(3,_): out of bound 1 (dimensions are 1x1)
error: called from
    corr at line 86 column 7
octave:4> corr(a',b')
ans = 1
octave:5> version -hgid
ans = ca72944d16a5
octave:6>


Dmitri,
--

Dmitri A. Sergatskov <dasergatskov>

 

(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 arungiridhar (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by nrjank
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by dasergatskov (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-07-17 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 9.1.0
    2023-07-10 nrjank Carbon-Copy- Added pr0
    2023-07-09 nrjank StatusNone Ready For Test
        Planned ReleaseNone 9.1.0

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code