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

abs tol unable to handle non-real numerical noise #36631

Closed
dimpase opened this issue Nov 2, 2023 · 2 comments · Fixed by #38433
Closed

abs tol unable to handle non-real numerical noise #36631

dimpase opened this issue Nov 2, 2023 · 2 comments · Fixed by #38433

Comments

@dimpase
Copy link
Member

dimpase commented Nov 2, 2023

E.g.

--- a/src/sage/doctest/parsing.py
+++ b/src/sage/doctest/parsing.py
@@ -1461,7 +1461,7 @@ class SageOutputChecker(doctest.OutputChecker):
             ...
             RuntimeError
             sage: 1  # abs tol 2
-            -0.5
+            -0.5+0.0001*I
             sage: print("0.9999")    # rel tol 1e-4
             1.0
             sage: print("1.00001")   # abs tol 1e-5

gives

sage -t --warn-long 20.5 --random-seed=37938279076849498617774420336933883040 src/sage/doctest/parsing.py
**********************************************************************
File "src/sage/doctest/parsing.py", line 1463, in sage.doctest.parsing.SageOutputChecker.check_output
Failed example:
    1  # abs tol 2
Expected:
    -0.5+0.0001*I
Got:
    1
**********************************************************************
1 item had failures:
   1 of  55 in sage.doctest.parsing.SageOutputChecker.check_output
    [322 tests, 1 failure, 0.17 s]
----------------------------------------------------------------------
sage -t --warn-long 20.5 --random-seed=37938279076849498617774420336933883040 src/sage/doctest/parsing.py  # 1 doctest failed

Indeed, RIF is used as data to check tolerances, no wonder things go wrong here.
By right, one needs a complex abs tol and rel tol.

Originally posted by @dimpase in #36509 (comment)

@jhpalmieri
Copy link
Member

jhpalmieri commented Nov 11, 2023

It's not precisely that it can't handle complex numerical noise, but there can't be a mismatch between the output type and the expected output type. One of the failing examples on OS X looks like

File "src/sage/tests/books/computational-mathematics-with-sagemath/mpoly_doctest.py", line 398, in sage.tests.books.computational-mathematics-with-sagemath.mpoly_doctest
Failed example:
    [CDF['x'](p(y=ys[0][0])).roots() for p in J.gens()] # abs tol 2e-15
Expected:
    [[(-0.5999999999999999, 1), (0.6000000000000001, 1)], [(0.6000000000000001, 1), (2.600000000000001, 1)]]
Got:
    [[(-0.5999999999999998, 1), (0.5999999999999999, 1)],
     [(0.6000000000000001 - 6.162975822039155e-33*I, 1),
      (2.6 + 3.0814879110195774e-32*I, 1)]]

If we change the expected output like this, it passses:

diff --git a/src/sage/tests/books/computational-mathematics-with-sagemath/mpoly_doctest.py b/src/sage/tests/books/computational-mathematics-with-sagemath/mpoly_doctest.py
index bef4a2b6c6..ab4be4fbbc 100644
--- a/src/sage/tests/books/computational-mathematics-with-sagemath/mpoly_doctest.py
+++ b/src/sage/tests/books/computational-mathematics-with-sagemath/mpoly_doctest.py
@@ -398,7 +398,7 @@ Sage example in ./mpoly.tex, line 1882::
   sage: ys = CDF['y'](Jy.0).roots(); ys # abs tol 2e-15
   [(-0.8, 1), (0.0, 1), (0.8, 1)]
   sage: [CDF['x'](p(y=ys[0][0])).roots() for p in J.gens()] # abs tol 2e-15
-  [[(-0.5999999999999999, 1), (0.6000000000000001, 1)], [(0.6000000000000001, 1), (2.600000000000001, 1)]]
+  [[(-0.5999999999999999, 1), (0.6000000000000001, 1)], [(0.6000000000000001 + 0*I, 1), (2.600000000000001 + 0*I, 1)]]
 
 Sage example in ./mpoly.tex, line 1911::
 

As far as I understand, the tolerance checker will check each real number in the output with the corresponding number that Sage actually produces, so if Sage produces a complex number (which gets parsed to a pair of reals), then the doctest has to actually provide a complex number (a pair of reals) to which to compare it. This is certainly a flaw in the doctesting procedure. It would be nice, at least for abs tol, if we could just take the absolute value of the difference between the expected output and the actual output and compare to the tolerance.

mkoeppe added a commit to dimpase/sage that referenced this issue Nov 18, 2023
dimpase pushed a commit to dimpase/sage that referenced this issue Nov 22, 2023
vbraun added a commit to vbraun/sage that referenced this issue Jul 27, 2024
For calculations over complex numbers that generate numeric noise, one
tends to create small but non-zero imaginary parts. This PR updates
the "# abs tol" tolerance setting to work over the complex numbers, as
the "abs" suggests complex numbers. The real and imaginary parts are
compared separately.

The ordinary "# tol" and "# rel tol" are left as is.

Fixes sagemath#36631
@vbraun
Copy link
Member

vbraun commented Jul 27, 2024

I've implemented the "complex abs tol" proposal in #38433, please review

vbraun added a commit to vbraun/sage that referenced this issue Jul 27, 2024
For calculations over complex numbers that generate numeric noise, one
tends to create small but non-zero imaginary parts. This PR updates
the "# abs tol" tolerance setting to work over the complex numbers, as
the "abs" suggests complex numbers. The real and imaginary parts are
compared separately.

The ordinary "# tol" and "# rel tol" are left as is.

Fixes sagemath#36631
vbraun added a commit to vbraun/sage that referenced this issue Jul 28, 2024
For calculations over complex numbers that generate numeric noise, one
tends to create small but non-zero imaginary parts. This PR updates
the "# abs tol" tolerance setting to work over the complex numbers, as
the "abs" suggests complex numbers. The real and imaginary parts are
compared separately.

The ordinary "# tol" and "# rel tol" are left as is.

Fixes sagemath#36631
vbraun added a commit to vbraun/sage that referenced this issue Jul 28, 2024
For calculations over complex numbers that generate numeric noise, one
tends to create small but non-zero imaginary parts. This PR updates
the "# abs tol" tolerance setting to work over the complex numbers, as
the "abs" suggests complex numbers. The real and imaginary parts are
compared separately.

The ordinary "# tol" and "# rel tol" are left as is.

Fixes sagemath#36631
vbraun pushed a commit to vbraun/sage that referenced this issue Aug 3, 2024
For calculations over complex numbers that generate numeric noise, one
tends to create small but non-zero imaginary parts. This PR updates the
"# abs tol" tolerance setting to work over the complex numbers, as the
"abs" suggests complex numbers. The real and imaginary parts are
compared separately.

The ordinary "# tol" and "# rel tol" are left as is.

Fixes sagemath#36631

URL: sagemath#38433
Reported by: Volker Braun
Reviewer(s): Dima Pasechnik
vbraun pushed a commit to vbraun/sage that referenced this issue Aug 5, 2024
For calculations over complex numbers that generate numeric noise, one
tends to create small but non-zero imaginary parts. This PR updates the
"# abs tol" tolerance setting to work over the complex numbers, as the
"abs" suggests complex numbers. The real and imaginary parts are
compared separately.

The ordinary "# tol" and "# rel tol" are left as is.

Fixes sagemath#36631

URL: sagemath#38433
Reported by: Volker Braun
Reviewer(s): Dima Pasechnik
vbraun pushed a commit to vbraun/sage that referenced this issue Aug 9, 2024
    
For calculations over complex numbers that generate numeric noise, one
tends to create small but non-zero imaginary parts. This PR updates the
"# abs tol" tolerance setting to work over the complex numbers, as the
"abs" suggests complex numbers. The real and imaginary parts are
compared separately.

The ordinary "# tol" and "# rel tol" are left as is.

Fixes sagemath#36631
    
URL: sagemath#38433
Reported by: Volker Braun
Reviewer(s): Dima Pasechnik
@vbraun vbraun closed this as completed in 1cca0f6 Aug 10, 2024
@mkoeppe mkoeppe added this to the sage-10.5 milestone Aug 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants