Fix inherently failing random_expr doctest

[rwst]

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]

comment:1

The lazy_import is not even necessary for trigger.

[rwst]

Description changed:

--- 
+++ 
@@ -47,4 +47,4 @@

The reason why the doctest fails is that, despite fixing the random seed, the functions and symbols picked by random_expr are different. This, at some point in the buildup of the expression, leads to the sub-expression kronecker_delta(-0.6165326641917295 + 0.15117833554974136*I, e) which immediately evaluates to 0. The 0 progresses eventually into a denominator, causing the error. random_expr is not written to avoid such errors if they happen, nor does it avoid other exceptions thrown. The author of the doctest apparently just chose a random seed that happens to not throw errors.

-Of course the actual bug is that random_expr changes behaviour when unrelated code is changed. +A fix that filters out zero divisions and intended exceptions thrown from code in functions before returning the expression would be the best solution (but it doesn't handle runtime errors from Pynac).

[rwst]

Description changed:

--- 
+++ 
@@ -1,50 +1,18 @@
-Applying the following minimal patch makes a completely unrelated doctest fail:
+The docs for `random_expr` read:

-diff --git a/src/sage/functions/all.py b/src/sage/functions/all.py -index e7f1ea0efa..d26f83f2f9 100644 ---- a/src/sage/functions/all.py -+++ b/src/sage/functions/all.py -@@ -4,7 +4,7 @@ from sage.misc.lazy_import import lazy_import

• lazy_import('sage.functions.piecewise_old', 'Piecewise') # deprecated
• lazy_import('sage.functions.piecewise', 'piecewise') --lazy_import('sage.functions.error', ['erf', 'erfc', 'erfi', 'erfinv']) -+lazy_import('sage.functions.error', ['erf', 'erfc', 'erfi', 'erfinv', 'fresnel_test1', 'fresnel_test2'])
• from .trig import ( sin, cos, sec, csc, cot, tan,
• asin, acos, atan, -diff --git a/src/sage/functions/error.py b/src/sage/functions/error.py -index 9a9b03cd6d..ec0a574814 100644 ---- a/src/sage/functions/error.py -+++ b/src/sage/functions/error.py -@@ -548,3 +548,15 @@ erfinv = Function_erfinv()
• from sage.structure.sage_object import register_unpickle_override
• register_unpickle_override('sage.functions.other', 'Function_erf', Function_erf)

• -+class Function_Fresnel_test1(BuiltinFunction): -+ def init(self): -+ BuiltinFunction.init(self, "fresnel_test1") -+ -+fresnel_test1 = Function_Fresnel_test1() -+ -+class Function_Fresnel_test2(BuiltinFunction): -+ def init(self): -+ BuiltinFunction.init(self, "fresnel_test2") -+ -+fresnel_test2 = Function_Fresnel_test2() -+

• 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:
  

  -File "src/sage/symbolic/random_tests.py", line 265, in sage.symbolic.random_tests.? -Failed example:

• random_expr(50, nvars=3, coeff_generator=CDF.random_element) # random
• 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/ ...
• File "sage/symbolic/expression.pyx", line 3518, in sage.symbolic.expression.Expression.div (build/cythonized/sage/symbolic/expression.cpp:25588)
• raise ZeroDivisionError("Symbolic division by zero")
• ZeroDivisionError: Symbolic division by zero

  
  -The reason why the doctest fails is that, despite fixing the random seed, the functions and symbols picked by `random_expr` are different. This, at some point in the buildup of the expression, leads to the sub-expression `kronecker_delta(-0.6165326641917295 + 0.15117833554974136*I, e)` which immediately evaluates to `0`. The `0` progresses eventually into a denominator, causing the error. `random_expr` is not written to avoid such errors if they happen, nor does it avoid other exceptions thrown. The author of the doctest apparently just chose a random seed that happens to not throw errors.
  +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. 

-A fix that filters out zero divisions and intended exceptions thrown from code in functions before returning the expression would be the best solution (but it doesn't handle runtime errors from Pynac). +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.

[rwst]

Branch: u/rws/fix_inherently_failing_random_expr_doctest

[rwst]

Commit: a17755c

[rwst]

Author: Ralf Stephan

[rwst]

New commits:

a17755c

24425: Fix inherently failing random_expr doctest

[mezzarobba]

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]

Reviewer: Marc Mezzarobba

[vbraun]

Changed branch from u/rws/fix_inherently_failing_random_expr_doctest to a17755c

[rwst]

Changed commit from a17755c to none

[rwst]

comment:8

Thanks. Oddly enough, #24212 depends on this.