#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
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 3 years ago by
comment:3 in reply to: ↑ 2 Changed 3 years ago by
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
- Reviewers set to Dima Pasechnik
- Status changed from needs_review to positive_review
over to bots...
comment:5 Changed 3 years ago by
- Priority changed from major to critical
Marking this as critical since it's a dependency of #22626
comment:6 Changed 3 years ago by
- 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
- 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.
OK, reviewing this probably needs a test run of ptestlong and ptest just to make sure nothing breaks. Looking into this now.