patchGNU Octave - Patches: patch #9989, [statistics] improve the...

 
 

patch #9989: [statistics] improve the compatibility of 'linkage'

Submitted by:  None
Submitted on:  Thu 29 Oct 2020 02:14:04 PM UTC  
 
Category:  Forge : other Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Originator Email:  -email is unavailable-
Open/Closed:  Closed

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)

Sun 03 Jan 2021 03:00:09 PM UTC, comment #8: 

comment #7:

> I had a brief look at cluster.m and inconsistent.m but that's too involved for now. I suppose k-means is used more widely.


OK, I submitted a couple of patches for those and, yes, that is mostly didactic stuff, while 'optimalleaforder' and others are more substantial omissions. 

Anonymous
Fri 01 Jan 2021 10:45:28 PM UTC, comment #7: 

Didn't see your last comment before I had fixed the warning and the Octave-7+ issue.
In the mean time my editor screwed up line endings => had to also fix that :-(

All in all a little more work than I initially hoped :-)

I had a brief look at cluster.m and inconsistent.m but that's too involved for now. I suppose k-means is used more widely.

Philip Nienhuis <philipnienhuis>
Project Member
Fri 01 Jan 2021 05:58:51 PM UTC, comment #6: 

Those two warnings are ok, it seems that the original author put a couple of tests for those warnings. Basically, when the method used is not functional, you got a series of cluster distances that do not increase monotonically and the function should notice you and you should re-run the function with a different method. There are two tests that should return that notice, which is a warning.

As for Octave 7, I have not installed it yet.

Anonymous
Fri 01 Jan 2021 03:55:49 PM UTC, comment #5: 

Thanks, I didn't first note that you uploaded a changeset rather than a "diff -u" as the extension suggested; all your details are inside. Fine.

While in current Octave-6.1.0 your patch doesn't influence the BIST tests, in dev Octave-7.0.0 it does:

>> test linkage
***** assert (cond (linkage (pdist (x))),              34.119045,t);
!!!!! test failed
: function called with too many inputs
shared variables   scalar structure containing the fields:
    x =
       0   1   4
       3   2   3
       1   2   2
       3   3   0
       0   4   4
       4   4   3
       1   1   4
       2   1   0
       4   2   0
       3   2   0
       0   2   1
       1   3   1
    t = 1.0000e-06
>>

In Octave-6.1.0 there's also a wrangled warning at L.177:

>> test linkage.m
warning: clustering
warning: called from
    linkage at line 177 column 5
    __test__ at line 3 column 2
    test at line 677 column 11

warning: clustering
warning: called from
    linkage at line 177 column 5
    __test__ at line 3 column 2
    test at line 677 column 11

PASSES 12 out of 12 tests

All in all it looks like linkage.m needs a bit more attention than just your patch.

Anyway I pushed your patch here:
http://hg.code.sf.net/p/octave/statistics/rev/48209fa818c1

Then, in your patch:

  • why is arg#4 called 'savememmory' rather than 'savememory'? Name and content of a variable don't bite each other if they're equal
  • the if clause has a typo": 'if (nargin == 5) ...' rather than 'if (nargin == 4) ...'
  • Please use '! <logical expression>' rather than  '~ <logical expression>'

I fixed those and pushed that here:
http://hg.code.sf.net/p/octave/statistics/rev/74d9b0baa81a

Again, thanks for your contribution.
Closing report.

Philip Nienhuis <philipnienhuis>
Project Member
Fri 01 Jan 2021 11:44:11 AM UTC, comment #4: 

Well, I am the same author as patch #9971 and I did not include my name in this one, because it is pretty simple. However feel free to credit me with my Sourceforge account -email is unavailable-

The 'cluster' function depends on the 'inconsistent' function which is also missing :-(

Anonymous
Thu 31 Dec 2020 07:28:35 PM UTC, comment #3: 

Yep, you're right; I applied the patch again to a new statistics package checkout and now it worked. That is, Octave crashed on the Matlab example, presumably because of OOM; but

>> X = rand (2000, 3);
>> Z = linkage (X, "ward");

... worked.
Now if only 'cluster' was implemented ... (then I could finish the Matlab example).

Would you like to be credited for the patch? then I can push it under your name, optionally with your email address (at least, that's the habit here). I'd only need your name then.

Philip Nienhuis <philipnienhuis>
Project Member
Thu 31 Dec 2020 04:17:45 PM UTC, comment #2: 

I think there must be a mistake. The patch I submitted removes that condition check on d:

   if (isempty (d))
     error ("linkage: d cannot be empty");
-  elseif ( nargin < 3 && ~ isvector (d))
-    error ("linkage: d must be a vector");
   endif

Actually, after applying the patch, you should be able to run:

>> X = rand (20000, 3);
>> Z = linkage (X, "ward");

That is exactly the main problem I tried to overcome with this patch. The other one being the "savememory" option.

Anonymous
Mon 28 Dec 2020 10:43:03 AM UTC, comment #1: 

Thanks for this patch. I had a brief look at it.

As I have little experience with the statistics package, the first thing I usually do when looking at patches for it here is trying the examples on the Matlab help page for the relevant function. All the more so if "compatibility" is mentioned in the title :-)
Now, with and without your patch the very first example there doesn't work in Octave:

>> rng ('default');
>> X = rand (20000, 3);
>> Z = linkage (X, "ward");
error: linkage: d must be a vector
error: called from
    linkage at line 102 column 5

... so there's a deeper problem with linkage as it stands.
Would you have any idea on how to make that example work in Octave?

Philip Nienhuis <philipnienhuis>
Project Member
Thu 29 Oct 2020 02:14:04 PM UTC, original submission:  

This is a patch to update the interface of the 'linkage' function to the status of its MATLAB counterpart.

Changes:
- call 'pdist' with less than 3 arguments, if the first one is a matrix
- throw a warning when the 'savememory' option is set, but do not stop the execution of the function

The 'savememory' option is not implemented, but since it does not change the output of the function it is not a big problem.

This patch does not break compatibility with older code.

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #50163:  update_linkage.diff added by None (2KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by None (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-01-01 philipnienhuis StatusNeed Info => Done
        Open/ClosedOpen => Closed
    2020-12-28 philipnienhuis StatusNone => Need Info
    2020-10-29 None Attached File- => Added update_linkage.diff, #50163

    Back to the top


    Powered by Savane 3.6