patchGNU Octave - Patches: patch #10060, [octave forge] (statistics) Update...

 
 

patch #10060: [octave forge] (statistics) Update function confusionchart

Submitted by:  None
Submitted on:  Sat 24 Apr 2021 08:56:26 PM UTC
 
Category:  Forge : new feature Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  philipnienhuis Originator Email:  -email is unavailable-
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       

 

( Jump to the original submission )

Tue 30 Nov 2021 08:06:18 AM UTC, comment #12: 

Right.
BTW I removed my email address from this patch report because apparently Savannah is confused - I got your last message 12 times.
Same for other bug & patch reports that are being worked on.

Philip Nienhuis <philipnienhuis>
Project Member
Tue 30 Nov 2021 12:36:08 AM UTC, comment #11: 

oh, actually it looks like you did apply v3.  so definitely all good. :)

Nicholas Jankowski <nrjank>
Project Member
Tue 30 Nov 2021 12:33:49 AM UTC, comment #10: 

it just trimmed a few lines of repeated code, but does the same thing.  v2 is fine.  thanks!

Nicholas Jankowski <nrjank>
Project Member
Mon 29 Nov 2021 10:13:16 PM UTC, comment #9: 

@Nick:
Too bad I was too eager :-)
If you think v3 is important please supply a cset here relative to the current repo.

Philip Nienhuis <philipnienhuis>
Project Member
Mon 29 Nov 2021 06:25:07 PM UTC, comment #8: 

OK pushed here:
http://hg.code.sf.net/p/octave/statistics/rev/a69a42fbdfcd

While testing (the demos) in Octave 8 (current dev) I hit a warning about deprecated '.-' so I changed those in the 6 occurrences / 4 files in the statistics package to '-':
http://hg.code.sf.net/p/octave/statistics/rev/337b67b11623

If you want you can ask Kai for push access.

Closing report.

Philip Nienhuis <philipnienhuis>
Project Member
Mon 29 Nov 2021 05:24:53 PM UTC, comment #7: 

another updated patch (v3) attached using a shared test variable to reduce the number of redundant lines.  verified it does preserve setting outside the test. not having the set lines does seem to result in global default changing when the tests work, so they're needed. but forcing certain errors inside the test blocks still results in the variable being correctly reset, so not sure how exactly that works.  Either way, this patch still seems to do the job.

(file #52383)

Nicholas Jankowski <nrjank>
Project Member
Mon 29 Nov 2021 02:53:58 PM UTC, comment #6: 

regarding best practices, I don't know if there's a better way to control this for tests.  If something did break halfway through, display would not be restored unless test already preserves that somehow.  I guess it wouldn't be a bad question over on the octave discourse

Nicholas Jankowski <nrjank>
Project Member
Mon 29 Nov 2021 02:51:52 PM UTC, comment #5: 

no push access. but attached is a v2 version of the second patch with a fixed commit message.

(file #52382)

Nicholas Jankowski <nrjank>
Project Member
Mon 29 Nov 2021 07:56:17 AM UTC, comment #4: 

Thanks Nick.

With tests I always wonder what will happen if it errors out somewhere, will settings introduced by it linger on and introduce unexpected behavior later on in the session?
Usually I fix / try to avoid that with some sort of "local" setting or a try-catch or so, but in case of graphics setting I wouldn't know how.

That aside, you patch (esp. 2nd one) is clearly an improvement. If you have push access, please do so and you can close this report then.
Otherwise I'll do but later today or this week.

Philip Nienhuis <philipnienhuis>
Project Member
Sun 28 Nov 2021 07:38:48 PM UTC, comment #3: 

@philip: I've attached two patches here to address the 'test confusion' figures issue.  It's solved by using a test block instead of 1-liner tests, and temporarily setting visibility to 'off' inside the block.

One patch just runs them all inside a single test block, turning the setting off and resetting it at the end.  This, however, only reports all pass/fails as a single test, which is sometimes deemed undesirable as you can't tell which test failed.

The second patch does the same thing, but instead breaks out each error test as an individual test block.  That's probably better from a test perspective, but maybe some people like the trimmer code. 

The difference between the two is essentially:

>> test confusionchart

PASSES 1 out of 1 test

>> test confusionchart
PASSES 18 out of 18 tests

feel free to apply whichever you prefer, either should apply clean to the latest tip.

(file #52366, file #52367)

Nicholas Jankowski <nrjank>
Project Member
Sat 24 Jul 2021 04:59:53 PM UTC, comment #2: 

Pushed your diff here (just edited the commit message a little):
http://hg.code.sf.net/p/octave/statistics/rev/09d06a2b75d0

I noted that "test confusionchart" leads to quite a few figures popping up and disappearing. I was wondering if we could do s/th with  "....., visibility, off" somewhere in the test section but TBH I don't know how do get that together cleanly.

OK, also here Status => "Ready For Test"

Philip Nienhuis <philipnienhuis>
Project Member
Sun 25 Apr 2021 10:02:39 AM UTC, comment #1: 

Thanks.

I guess I should pick this up as I did with most of your other patches lately, but I've got very little time for Octave these days. Just hang on & be patient, please :-)

Philip Nienhuis <philipnienhuis>
Project Member
Sat 24 Apr 2021 08:56:26 PM UTC, original submission:  

A few more options for function 'confusionchart' to improve its compatibility.

Most notably this patch adds the "HandleVisibility" property to the ConfusionMatrixChart object. Besides that, it adds the "Position", "OuterPosition" and "Units" properties.

For the new properties there are also new tests.

Anonymous

 

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

Attach Files:
   
   
Comment:
   

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by nrjank
  • -email is unavailable- added by nrjank (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 logged-in users can vote.

     

     

     

     

    Follow 18 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-11-30 philipnienhuis Carbon-CopyRemoved philipnienhuis => -
    2021-11-29 philipnienhuis StatusReady For Test => Done
        Open/ClosedOpen => Closed
    2021-11-29 nrjank Attached File- => Added octave-statistics-confusionchart-suppresstestfigs_v3.patch, #52383
    2021-11-29 nrjank Attached File- => Added octave-statistics-confusionchart-suppresstestfigs_v2.patch, #52382
    2021-11-28 nrjank Attached File- => Added octave-statistics-confusionchart-suppresstestfigs_1test.patch, #52366
        Attached File- => Added octave-statistics-confusionchart-suppresstestfigs_manytests.patch, #52367
        StatusDone => Ready For Test
        Open/ClosedClosed => Open
        Carbon-Copy- => Added nir_krakauer
    2021-11-23 philipnienhuis StatusReady For Test => Done
        Open/ClosedOpen => Closed
    2021-11-23 nrjank CategoryNone => Forge : new feature
    2021-07-24 philipnienhuis StatusIn Progress => Ready For Test
    2021-07-24 philipnienhuis StatusNone => In Progress
        Assigned toNone => philipnienhuis
        Carbon-Copy- => Added philipnienhuis
    2021-04-24 None Attached File- => Added confusionchart_update.diff, #51315

    Back to the top


    Powered by Savane 3.9