Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the randgen framework to set the seeds for controlled Magma, Singular, etc. sessions #3231

Closed
malb opened this issue May 16, 2008 · 46 comments

Comments

@malb
Copy link
Member

malb commented May 16, 2008

The interfaces to gp, gap, r, scilab, magma, etc., should use the randgen framework to initialize/set the seed for their respective random number generators.

The purpose of this ticket is to add a set_seed method to each interface which takes the appropriate input to modify the random number generator's seed value. It should also make the interfaces initialize their seed value with a random value generated from the randgen framework.

Component: interfaces

Author: Travis Scholl

Branch/Commit: 38256ae

Reviewer: Martin Albrecht

Issue created by migration from https://trac.sagemath.org/ticket/3231

@malb malb added this to the sage-5.11 milestone May 16, 2008
@sagetrac-cwitty sagetrac-cwitty mannequin assigned sagetrac-cwitty and unassigned williamstein Feb 24, 2009
@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@tscholl2
Copy link
Contributor

Commit: 141e8e9

@tscholl2
Copy link
Contributor

New commits:

e532a6cadded set_seed support for singular
aa2b61bused randstate to initialize seed in some interfaces
141e8e9Added set_seed functionality to more interfaces.

@tscholl2
Copy link
Contributor

Author: Travis Scholl

@tscholl2
Copy link
Contributor

Branch: u/tscholl2/seeds_3231

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 26, 2015

Changed commit from 141e8e9 to 33abf28

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 26, 2015

Branch pushed to git repo; I updated commit sha1. New commits:

33abf28Added Magma example

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 26, 2015

Branch pushed to git repo; I updated commit sha1. New commits:

3f8533afixed randstate singular example

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 26, 2015

Changed commit from 33abf28 to 3f8533a

@tscholl2
Copy link
Contributor

comment:10

I added a set_seed function on a lot of the interfaces such as gap, gp, magma, maxima, R, and singular. This allows setting the seed for the corresponding random number generator.

This made solving the ticket very easy by just calling "set_seed" when the interface is started with _start.

I think someone should rewrite the randstate class to use these methods if possible. However that would require someone with more cython knowledge.

Also set_seed still needs to be implemented on a few more interfaces, but those are not free so they are not as easy to test with.

@malb
Copy link
Member Author

malb commented May 27, 2015

comment:11

Maybe this ticket should be repurposed then and another ticket created to realise the integration into the randgen() framework? It'd be a shame if this bitrots.

@tscholl2
Copy link
Contributor

comment:12

I think this ticket is fine because it's still a step in the right direction. In my mind the best framework would be for every interface to have a set_seed function like these. Then randstate would have methods that take in an instance of an interface and set it's seed using that method. Right now there is no control over multiple instances of interfaces, which it seems like there could/should be, and it might even simplify some of the randstate code.

Also randstate would still set seeds for non-interfaces such as libraries like libc and python.random (or wrap the random calls as it does). All of the set_seed methods I wrote default to interface.rand_seed on startup which uses randstate, so I think this fit the description of this ticket.

I think the next ticket should be specifically about randstate and be dependent on this one. I am worried repurposing this ticket to do both might be a bit much. Or did you have something else in mind?

@malb
Copy link
Member Author

malb commented May 28, 2015

comment:13

I agree: let's deal with what you've done in this ticket (and change the description accordingly) and then open another follow up ticket which deals with the randstate business.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 29, 2015

Branch pushed to git repo; I updated commit sha1. New commits:

8601eb9reverted changes in randstate

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 29, 2015

Changed commit from 3f8533a to 8601eb9

@tscholl2
Copy link
Contributor

comment:27

I added a doc test for the set_seed method for the general interface. It was easy to add.

@malb
Copy link
Member Author

malb commented Jun 15, 2015

comment:28

Looks good to me.

@vbraun
Copy link
Member

vbraun commented Jun 15, 2015

comment:29
sage -t --long src/sage/interfaces/magma.py  # 2 doctests failed
sage -t --long src/sage/interfaces/octave.py  # 2 doctests failed
sage -t --long src/sage/interfaces/scilab.py  # 2 doctests failed
sage -t --long src/sage/repl/interpreter.py  # 1 doctest failed

@tscholl2
Copy link
Contributor

comment:30

I get the first 3 errors if I run sage -t --long src/sage/interfaces/magma.py but not if I run ./sage -t --long src/sage/interfaces/magma.py. Shouldn't I only be checking if it runs with the version of Sage on the branch and not the user's systems' version?

The last error seems to be something I didn't notice. Apparently some of the interfaces keep a counter of the commands used and label things with it. The set_seed function for some interfaces raises this counter because it needs to call a command in the interface. So running the test in src/sage/repl/interpreter.py the counter is off from what it used to be.

This error doesn't occur if I comment out line 626 in src/sage/interfaces/maxima.py:

625 # set random seed
626 self.set_seed(self._seed)

I'm not sure what to do about this. It seems like there is a few options:

  • We could reset the counter after the set_seed function is called (I actually don't know if this is possible yet).
  • We could change all the doc tests in functions which use this counter and increment it manually.
  • We could not run set_seed on start and leave it up to the user to run it.

Martin, do you have any suggestions on which way to proceed?

@malb
Copy link
Member Author

malb commented Jun 18, 2015

comment:31

Ah, crap my comments from yesterday seem not to have arrived:

  • You are not seeing these errors because you have octave et al. installed in your local development environment. In other words: some doctests are not marked optional but you should be marked optional.

  • I'd just update the doctest, set_seed() is a proper call, so no point in resetting the counter.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 18, 2015

Changed commit from 773a1f3 to df73e17

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 18, 2015

Branch pushed to git repo; I updated commit sha1. New commits:

df73e17added # optional to set_seed doc tests

@tscholl2
Copy link
Contributor

comment:33

I added # optional - tags to the corresponding doc tests. I think I got all of them, but someone should check. Strangely enough, this also fixed the other test sage -t --long src/sage/repl/interpreter.py. Which may mean that either it wasn't actually counting the calls to the interpreter, or something else happened with the optional flags which affected it.

@malb
Copy link
Member Author

malb commented Jun 19, 2015

comment:34

I still get

sage -t --warn-long 33.6 src/sage/repl/interpreter.py  # 1 doctest failed
**********************************************************************
File "src/sage/repl/interpreter.py", line 561, in sage.repl.interpreter.InterfaceShellTransformer.preparse_imports_from_sage
Failed example:
    ift.preparse_imports_from_sage('2 + maxima(a)')
Expected:
    '2 +  sage1 '
Got:
    '2 +  sage4 '

but the other three are gone.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 21, 2015

Changed commit from df73e17 to 38256ae

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 21, 2015

Branch pushed to git repo; I updated commit sha1. New commits:

38256aechanged doc test because set_seed call

@tscholl2
Copy link
Contributor

comment:36

I removed some white space and changed a doctest in src/sage/repl/interpreter.py.

The doc test was failing because maxima now calls set_seed on startup which uses several calls to the maxima interpreter. Therefore instead of getting back 2 + sage1, it should get back 2 + sage4.

@malb
Copy link
Member Author

malb commented Jun 22, 2015

comment:37

tests pass here & looks good.

@vbraun
Copy link
Member

vbraun commented Jun 22, 2015

Changed branch from u/tscholl2/seeds_3231 to 38256ae

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants