Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#24425 closed defect (fixed)

Fix inherently failing random_expr doctest

Reported by: rws Owned by:
Priority: major Milestone: sage-8.2
Component: symbolics Keywords:
Cc: Merged in:
Authors: Ralf Stephan Reviewers: Marc Mezzarobba
Report Upstream: N/A Work issues:
Branch: a17755c (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by rws)

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.

Change History (8)

comment:1 Changed 5 years ago by rws

The lazy_import is not even necessary for trigger.

comment:2 Changed 5 years ago by rws

Description: modified (diff)
Summary: random_expr changes behaviour when unrelated code is changedMinimize random_expr's raising exceptions

comment:3 Changed 5 years ago by rws

Description: modified (diff)
Summary: Minimize random_expr's raising exceptionsFix inherently failing random_expr doctest

comment:4 Changed 5 years ago by rws

Branch: u/rws/fix_inherently_failing_random_expr_doctest

comment:5 Changed 5 years ago by rws

Authors: Ralf Stephan
Commit: a17755c56a524033d3075a737f4a08facd012dbb
Status: newneeds_review

New commits:

a17755c24425: Fix inherently failing random_expr doctest

comment:6 Changed 5 years ago by mmezzarobba

Reviewers: Marc Mezzarobba
Status: needs_reviewpositive_review

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

Version 0, edited 5 years ago by mmezzarobba (next)

comment:7 Changed 5 years ago by vbraun

Branch: u/rws/fix_inherently_failing_random_expr_doctesta17755c56a524033d3075a737f4a08facd012dbb
Resolution: fixed
Status: positive_reviewclosed

comment:8 Changed 5 years ago by rws

Commit: a17755c56a524033d3075a737f4a08facd012dbb

Thanks. Oddly enough, #24212 depends on this.

Note: See TracTickets for help on using tickets.