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

Make combinat doctests ready for random seeds #29974

Closed
kliem opened this issue Jun 24, 2020 · 30 comments
Closed

Make combinat doctests ready for random seeds #29974

kliem opened this issue Jun 24, 2020 · 30 comments

Comments

@kliem
Copy link
Contributor

kliem commented Jun 24, 2020

Part of #29935.

This ticket makes

sage -t --long --random-seed=n src/sage/combinat/

pass for n more general than just 0.

See also:

Along the way we fix a tiny but annoying bug, where random derangements could fail.

That was because randint(0, n) has range range(0, n + 1),
which had caused confusion.

Depends on #29962

Component: doctest framework

Author: Jonathan Kliem

Branch: 928863f

Reviewer: Sébastien Labbé

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

@kliem kliem added this to the sage-9.2 milestone Jun 24, 2020
@kliem
Copy link
Contributor Author

kliem commented Jun 24, 2020

Branch: public/29974

@kliem
Copy link
Contributor Author

kliem commented Jun 24, 2020

New commits:

da1c6bestart from a "random" random seed for doctesting
b7b836dmake random seed reproducible
eedbe5edocument random_seed
998b1b9default random seed 0 for now
1d7b00edash instead of underscore for command line options
e0e572emake combinat fuzz ready
7c3a1ffanother file for combinat

@kliem
Copy link
Contributor Author

kliem commented Jun 24, 2020

Commit: 7c3a1ff

@kliem
Copy link
Contributor Author

kliem commented Jul 12, 2020

comment:2

Merge conflict.

I also need to go through it again and check I respected the design decision in #29935.

@kliem
Copy link
Contributor Author

kliem commented Jul 20, 2020

comment:3

Note that elements of g = GelfandTsetlinPatterns(4, 5) have the wrong parent.

@kliem

This comment has been minimized.

@kliem
Copy link
Contributor Author

kliem commented Jul 21, 2020

Changed commit from 7c3a1ff to none

@kliem
Copy link
Contributor Author

kliem commented Jul 21, 2020

Changed branch from public/29974 to public/29974-reb

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 21, 2020

Commit: a914b8d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 21, 2020

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

eedbe5edocument random_seed
998b1b9default random seed 0 for now
1d7b00edash instead of underscore for command line options
b31e2d5Merge branch 'public/29962' of git://trac.sagemath.org/sage into public/29962-reb
2f30dd9small fixes
b62f781doctests do not start from a random seed by default yet
1d99129fix merge conflict
d40d826make combinat fuzz ready
b9f88cfanother file for combinat
a914b8dmodify doctests according to decisions in 29935

@seblabbe
Copy link
Contributor

comment:8

I get one failure in combinat:

----------------------------------------------------------------------
sage -t --long --random-seed=2 src/sage/combinat/derangements.py  # 1 doctest failed
----------------------------------------------------------------------

Can you reproduce it?

sage -t --long --random-seed=2 src/sage/combinat/derangements.py
**********************************************************************
File "src/sage/combinat/derangements.py", line 129, in sage.combinat.derangements.Derangements
Failed example:
    D2.random_element() # random
Exception raised:
    Traceback (most recent call last):
      File "/home/slabbe/GitBox/sage/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 715, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/slabbe/GitBox/sage/local/lib/python3.7/site-packages/sage/doctest/forker.py", line 1139, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.combinat.derangements.Derangements[7]>", line 1, in <module>
        D2.random_element() # random
      File "/home/slabbe/GitBox/sage/local/lib/python3.7/site-packages/sage/combinat/derangements.py", line 504, in random_element
        return L[i]
    IndexError: list index out of range
**********************************************************************
1 item had failures:
   1 of   9 in sage.combinat.derangements.Derangements
    [85 tests, 1 failure, 0.10 s]

@kliem
Copy link
Contributor Author

kliem commented Aug 14, 2020

comment:9

Yes, I can confirm the problem.

@seblabbe
Copy link
Contributor

Reviewer: Sébastien Labbé

@seblabbe
Copy link
Contributor

comment:10

But it works for --random-seed=0, --random-seed=1, --random-seed=3, --random-seed=4.

@kliem
Copy link
Contributor Author

kliem commented Aug 15, 2020

Changed branch from public/29974-reb to public/29974-reb2

@kliem
Copy link
Contributor Author

kliem commented Aug 15, 2020

comment:11

Tiny mistake that the range of randint(0,n) is actually range(0,n+1).


New commits:

48bd40eMerge branch 'public/29974-reb' of git://trac.sagemath.org/sage into test_29974
39f94e6fix index error with random derangements

@kliem
Copy link
Contributor Author

kliem commented Aug 15, 2020

Changed commit from a914b8d to 39f94e6

@kliem

This comment has been minimized.

@seblabbe
Copy link
Contributor

comment:12

There is

from random import randrange

for that. Do you mind to change to the lesser known randrange?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 15, 2020

Changed commit from 39f94e6 to a391a3e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 15, 2020

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

a391a3euse randrange instead of randint

@kliem
Copy link
Contributor Author

kliem commented Aug 15, 2020

comment:14

I like randrange much better. It is much more intuitive if you are used to range. (But reading the very short docstring that was exactly the point of introducing it.)

@seblabbe
Copy link
Contributor

comment:15

In fact, I think we want to import it from prandom

from sage.misc.prandom import randrange  

in order for it to depend on the --random-seed.

@kliem
Copy link
Contributor Author

kliem commented Aug 15, 2020

comment:16

I was wondering about what the point was of duplicating that thing.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 15, 2020

Changed commit from a391a3e to 928863f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 15, 2020

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

928863fimport randrange from the correct place

@kliem
Copy link
Contributor Author

kliem commented Aug 15, 2020

comment:19

Thank you.

@vbraun
Copy link
Member

vbraun commented Aug 16, 2020

Changed branch from public/29974-reb2 to 928863f

@slel

This comment has been minimized.

@slel
Copy link
Member

slel commented Apr 29, 2021

Changed commit from 928863f to none

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

4 participants