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

Fix inherently failing random_expr doctest #24425

Closed
rwst opened this issue Dec 24, 2017 · 12 comments
Closed

Fix inherently failing random_expr doctest #24425

rwst opened this issue Dec 24, 2017 · 12 comments

Comments

@rwst
Copy link
Contributor

rwst commented Dec 24, 2017

The docs for random_expr read:

  This function will often raise an error because it tries to create
  an erroneous expression (such as a division by zero).

It has the following doctest:

        sage: from sage.symbolic.random_tests import *
        sage: set_random_seed(53)
        sage: random_expr(50, nvars=3, coeff_generator=CDF.random_element) # random
        (v1^(0.97134084277 + 0.195868299334*I)/csc(-pi + v1^2 + v3) + sgn(1/
...

Despite having a random seed the test run changes with every new builtin function introduced in sage/functions because the global function list changes. That's why the test was marked random. The problem however is that the test can even raise an error, as the docs state above. The"random" keyword does not catch this, and it would make the test useless anyway.

Tests are meant to test the functionality of the associated code so the test and perhaps random_expr should be rewritten such that it allows a test that does not change with a changed global function list.

Component: symbolics

Author: Ralf Stephan

Branch: a17755c

Reviewer: Marc Mezzarobba

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

@rwst rwst added this to the sage-8.2 milestone Dec 24, 2017
@rwst
Copy link
Contributor Author

rwst commented Dec 24, 2017

comment:1

The lazy_import is not even necessary for trigger.

@rwst

This comment has been minimized.

@rwst rwst changed the title random_expr changes behaviour when unrelated code is changed Minimize random_expr's raising exceptions Dec 24, 2017
@rwst

This comment has been minimized.

@rwst rwst changed the title Minimize random_expr's raising exceptions Fix inherently failing random_expr doctest Dec 26, 2017
@rwst
Copy link
Contributor Author

rwst commented Dec 26, 2017

@rwst
Copy link
Contributor Author

rwst commented Dec 26, 2017

Commit: a17755c

@rwst
Copy link
Contributor Author

rwst commented Dec 26, 2017

Author: Ralf Stephan

@rwst
Copy link
Contributor Author

rwst commented Dec 26, 2017

New commits:

a17755c24425: Fix inherently failing random_expr doctest

@mezzarobba
Copy link
Member

comment:6

I don't fully understand the context, but the change looks reasonable and really unlikely to break anything—and this ticket has been languishing for months.

@mezzarobba
Copy link
Member

Reviewer: Marc Mezzarobba

@vbraun
Copy link
Member

vbraun commented Jun 2, 2018

Changed branch from u/rws/fix_inherently_failing_random_expr_doctest to a17755c

@rwst
Copy link
Contributor Author

rwst commented Jun 2, 2018

Changed commit from a17755c to none

@rwst
Copy link
Contributor Author

rwst commented Jun 2, 2018

comment:8

Thanks. Oddly enough, #24212 depends on this.

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

3 participants