bugGNU Octave - Bugs: bug #53254, tests: several unit tests set a...

 
 

bug #53254: tests: several unit tests set a random "seed" or "state" without restoring it

Submitter:  Mike Miller <mtmiller>
Submitted:  Thu 01 Mar 2018 08:05:33 PM UTC
   
 
Category:  Test Suite Severity:  4 - Important
Priority:  5 - Normal Item Group:  Inaccurate Result
Status:  Fixed Assigned to:  mtmiller
Originator Name:  Open/Closed:  * Closed
Release:  * 4.2.1 Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Fri 02 Mar 2018 03:29:27 AM UTC, comment #5: 

@Mike <<<This may expose some more tolerance errors in some functions in the future>>>

This did not take long:


http://buildbot.octave.org:8010/builders/gcc-fedora/builds/1353/steps/test/logs/stdio

>>>>> processing /home/buildbotu/fc25-x86_64/gcc-fedora/src/scripts/sparse/pcg.m
***** test
 ## solve small diagonal system

 N = 10;
 A = diag ([1:N]); b = rand (N, 1);
 X = A \ b;  # X is the true solution
 [x, flag] = pcg (A, b, [], N+1);
 assert (norm (x - X) / norm (X), 0, 1e-10);
 assert (flag, 0);
!!!!! test failed
ASSERT errors for:  assert (norm (x - X) / norm (X),0,1e-10)

  Location  |  Observed  |  Expected  |  Reason
     ()       3.9945e-07       0         Abs err 3.9945e-07 exceeds tol 1e-10 by 4e-07


Looks pretty gross. I will file a separate bug report.

Dmitri A. Sergatskov <dasergatskov>
Fri 02 Mar 2018 02:49:59 AM UTC, comment #4: 

I pushed a fix on stable and default for this issue.

https://hg.savannah.gnu.org/hgweb/octave/rev/261d23b8933b

I was able to add some code to '__run_test_suite__' to print all random state values, and diff the outputs from run to run. Now every run of 'make check' does have a different random state from start to finish.

This may expose some more tolerance errors in some functions in the future, since all tests under scripts and test using any rand function were being run with the same values every time until now.

Mike Miller <mtmiller>
Group Member
Thu 01 Mar 2018 11:04:28 PM UTC, comment #3: 

I have a partial fix for the problems in conv2.cc, eigs.m, and median.m.

For conv2.cc, unfortunately, I have to use unwind_protect because it needs to restore both "seed" and "state" in the correct order. The fact that I can't get the test to pass at all by trying to switch it to rand('state', ...) probably means that the test is extremely fragile and should probably be rewritten anyway. I'm adding a FIXME describing that for later.

I am now looking at rand.cc, which exhaustively checks all the rand* functions (as it should), and returns with every random generator in a fixed state.

Mike Miller <mtmiller>
Group Member
Thu 01 Mar 2018 08:31:33 PM UTC, comment #2: 

I like the onCleanup method because it feels more concise than the unwind_protect blocks.


Rik <rik5>
Group administrator
Thu 01 Mar 2018 08:29:16 PM UTC, comment #1: 

Since the same problem affects the stable branch as well, I'm looking at fixing the tests in conv2.cc, eigs.m, and median.m on the stable branch.

I am also going to try to put some debug into the test suite script to try to audit that the random state is different in every run, that may take a little while.

Mike Miller <mtmiller>
Group Member
Thu 01 Mar 2018 08:05:33 PM UTC, original submission:  

This is a test suite QA issue. Several of the unit tests set the random generator "state" value without restoring it, or even worse, call rand("seed", x), which according to the docs switches the internal algorithm, without switching it back to the default.

This means all tests that follow the test in question and asking for random values are not really getting random values, and may be getting values from the old generator algorithm.

Through grepping, the following function unit tests have a call to rand setting the "seed" or "state" value and not setting it back (additionally setting the algorithm back if calling "seed"):

  • libinterp/corefcn/conv2.cc
  • scripts/sparse/eigs.m
  • scripts/statistics/median.m


It would not hurt to look at other functions that set and restore the random generator state to make sure they are doing it safely and consistently. Some functions use an unwind_protect block and some use an onCleanup function, for example.

It wouldn't hurt to also double check the tests in libinterp/corefcn/rand.cc to make sure they are doing the right thing.

Mike Miller <mtmiller>
Group Member

 

(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 dasergatskov (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by mtmiller (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
    2018-03-02 mtmiller StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2018-03-01 mtmiller StatusConfirmed In Progress
        Assigned toNone mtmiller
        Releasedev 4.2.1
    2018-03-01 rik5 StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code