Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#26874 closed defect (fixed)

Use current_randstate() for seeding PRNGs used by Interfaces

Reported by: embray Owned by:
Priority: critical Milestone: sage-8.6
Component: doctest framework Keywords:
Cc: dimpase, jdemeyer Merged in:
Authors: Erik Bray Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: d7c85cd (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

There is a method Interface.rand_seed() which returns a randomized seed that can be used to initialize different Interfaces RNGs, where applicable (it use called by individual interface implementations' set_seed() methods when the provided seed is None).

For some reason (itself not entirely clear to me) this does not use the current_randstate(), but rather initializes a new randstate() and uses that to obtain an arbitrary random seed. Maybe there's no good reason for that in the first place, in fact?

In any case, for the doctests we want consistent results, so with this patch I have this method use current_randstate().seed() so that results are consistent across test runs.

This came up in comment:341:ticket:22626 where it turned out some tests that use the GAP pexpect interface which depend on GAP's PRNG were different between test runs.

There are many other tests like this sprinkled about which manage this by manually calling current_randstate().set_seed_gap() before the test, but putting something like this in the documentation would seem rather distracting.

Change History (7)

comment:1 Changed 3 years ago by embray

  • Status changed from new to needs_review

comment:2 follow-up: Changed 3 years ago by dimpase

OK, reviewing this probably needs a test run of ptestlong and ptest just to make sure nothing breaks. Looking into this now.

comment:3 in reply to: ↑ 2 Changed 3 years ago by embray

Replying to dimpase:

OK, reviewing this probably needs a test run of ptestlong and ptest just to make sure nothing breaks. Looking into this now.

Already did last night. Remarkably, nothing broke. There probably isn't actually all that much that depends on this, and most tests that could have been affected by it were already manually calling current_randstate().set_seed_<foo>() ahead of time...

comment:4 Changed 3 years ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

over to bots...

comment:5 Changed 3 years ago by embray

  • Priority changed from major to critical

Marking this as critical since it's a dependency of #22626

comment:6 Changed 3 years ago by vbraun

  • Branch changed from u/embray/doctests/interfaces-rand-seed to d7c85cd7ba758f5661245b994d857ca2a22f1637
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:7 Changed 3 years ago by embray

  • Commit d7c85cd7ba758f5661245b994d857ca2a22f1637 deleted
  • Milestone changed from sage-8.5 to sage-8.6

Closed 4 days ago so I'm assuming this didn't go in 8.5. Please don't close a ticket as fixed without updating the milestone to its target release.

Note: See TracTickets for help on using tickets.