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

disable a very long doctest in omega.py #23865

Closed
videlec opened this issue Sep 15, 2017 · 19 comments
Closed

disable a very long doctest in omega.py #23865

videlec opened this issue Sep 15, 2017 · 19 comments

Comments

@videlec
Copy link
Contributor

videlec commented Sep 15, 2017

Disable the following from sage/rings/polynomial/omega.py that takes forever without any justification

sage: from sage.rings.polynomial.omega import Omega_ge
sage: %time Omega_ge(0, (2, 2, 1, 1, 1, 1, 1, -1, -1))[0].number_of_terms()
CPU times: user 53.1 s, sys: 66 ms, total: 53.2 s
Wall time: 53.1 s
27837

(running the doctests in omega.py takes longer than running the doctests in all other files from sage/rings/polynomial/)

CC: @dkrenn

Component: documentation

Author: Daniel Krenn

Branch/Commit: 12734e4

Reviewer: Daniel Krenn, Vincent Delecroix

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

@videlec videlec added this to the sage-8.1 milestone Sep 15, 2017
@videlec
Copy link
Contributor Author

videlec commented Sep 15, 2017

Branch: u/vdelecroix/23865

@videlec
Copy link
Contributor Author

videlec commented Sep 15, 2017

New commits:

f62bf2723865: disable doctest in omega

@videlec
Copy link
Contributor Author

videlec commented Sep 15, 2017

Commit: f62bf27

@jdemeyer
Copy link

comment:2

Instead of disabling it, could the test be simplified?

@videlec
Copy link
Contributor Author

videlec commented Sep 15, 2017

comment:3

This is why I put dkrenn in copy! I am not exactly sure what this doctest is meant for.

@dkrenn
Copy link
Contributor

dkrenn commented Sep 19, 2017

comment:4

Replying to @jdemeyer:

Instead of disabling it, could the test be simplified?

Yes, this can be simplified. I'll provide a corrected version in the next days.

@dkrenn
Copy link
Contributor

dkrenn commented Sep 19, 2017

Replying to @videlec:

Disable the following from sage/rings/polynomial/omega.py that takes forever without any justification

The motivation for having this is, that there is a separate computation for the result meaning that the result is somehow verified. I do not recall that it took that long and will investigate.

@videlec
Copy link
Contributor Author

videlec commented Sep 19, 2017

comment:6

Thanks Daniel!

@dkrenn
Copy link
Contributor

dkrenn commented Sep 19, 2017

Changed branch from u/vdelecroix/23865 to u/dkrenn/23865

@dkrenn
Copy link
Contributor

dkrenn commented Sep 19, 2017

comment:8

Small reviewer patch added (untested --> not tested) and I've added a new, shorter doctest which needs a small cross review. [Good to go from my side.]


New commits:

1401a2423865: untested --> not tested
12734e423865: add one long time doctest

@dkrenn
Copy link
Contributor

dkrenn commented Sep 19, 2017

Changed author from Vincent Delecroix to Vincent Delecroix, Daniel Krenn

@dkrenn
Copy link
Contributor

dkrenn commented Sep 19, 2017

Changed commit from f62bf27 to 12734e4

@dkrenn
Copy link
Contributor

dkrenn commented Sep 19, 2017

Reviewer: Daniel Krenn

@dkrenn
Copy link
Contributor

dkrenn commented Sep 19, 2017

comment:9

FYI, all non-long tests in the file now take 1 second; including the long tests 2 seconds.

@videlec
Copy link
Contributor Author

videlec commented Sep 19, 2017

Changed author from Vincent Delecroix, Daniel Krenn to Daniel Krenn

@videlec
Copy link
Contributor Author

videlec commented Sep 19, 2017

comment:10

Indeed, on the patchbot it is now

sage -t --long src/sage/rings/polynomial/omega.py
    [127 tests, 1.84 s]

Thanks!

@videlec
Copy link
Contributor Author

videlec commented Sep 19, 2017

Changed reviewer from Daniel Krenn to Daniel Krenn, Vincent Delecroix

@videlec
Copy link
Contributor Author

videlec commented Oct 4, 2017

comment:11

oups. I forgot to positive review it...

@vbraun
Copy link
Member

vbraun commented Oct 5, 2017

Changed branch from u/dkrenn/23865 to 12734e4

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