Opened 2 years ago

Closed 17 months ago

#28517 closed defect (fixed)

Make the evaluation of symbolic functions on intervals more reliable

Reported by: mmezzarobba Owned by:
Priority: critical Milestone: sage-9.2
Component: numerical Keywords:
Cc: Merged in:
Authors: Marc Mezzarobba Reviewers: Vincent Delecroix, Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: e7a4959 (Commits, GitHub, GitLab) Commit: e7a49599cac8649068c0c966b427c621224f7a3c
Dependencies: #28469, #28508, #28516 Stopgaps:

Status badges

Description (last modified by mmezzarobba)

Fix the following *incorrect results*, and some related ones, either by returning a correct result or by raising an error instead.

sage: b = RBF(3/2, 1e-10)
sage: gamma(b, 1)
[0.5072822338117733 +/- 1.35e-17]
sage: gamma(CBF(b), 1)
[0.50728223381177 +/- 6.57e-15] + [3.505711e-11 +/- 2.19e-18]*I
sage: hurwitz_zeta(b, b)
[1.948110822808643 +/- 2.28e-16]
sage: hurwitz_zeta(1/2, b)
[-2.019112205794726 +/- 5.00e-16]
sage: airy_ai(b)
[0.07174949700810541 +/- 4.54e-18]

sage: iv = RIF(1, 1.0001)
sage: airy_ai(iv)
0.13528445910993159?
sage: airy_ai(CIF(iv))
0.13528445910993159?

sage: CBF(RBF(airy_ai(1))).overlaps(ComplexBallField(100).one().airy_ai())
False
sage: CBF(airy_ai(1)).overlaps(ComplexBallField(100).one().airy_ai())
False
sage: RBF(zetaderiv(1, 3/2)).overlaps(RealBallField(100)(3/2).zetaderiv(1))
False
sage: CBF(zetaderiv(1, 3/2)).overlaps(ComplexBallField(100)(3/2).zetaderiv(1))
False

Remarks:

  • The cases where we now raise an error could be improved later by implementing/interfacing actual interval implementations of the relevant functions and/or adding general fallbacks mechanisms to evaluate functions on elements of RIF/CIF/RBF (and even RR, CC) based on the implementations available for CBF.
  • This is little more than a band-aid solution, because much of the symbolic expression/function code manifestly follows the philosophy that a slightly wrong result is better than no result at all. (And some of the issues are in Pynac, not in Sage.)
  • Nevertheless, this ticket has accumulated a number of small fixes that make them significantly less buggy...

See also the individual commit messages for more information.

Change History (38)

comment:1 Changed 2 years ago by mmezzarobba

  • Authors set to Marc Mezzarobba
  • Branch set to u/mmezzarobba/symbolic_functions_of_intervals
  • Commit set to 6323053dd30a3cf54fc99833a7a423c6472c14c8
  • Dependencies set to #28469, #28508, #28516
  • Description modified (diff)
  • Status changed from new to needs_review
Last edited 2 years ago by mmezzarobba (previous) (diff)

comment:2 Changed 2 years ago by mmezzarobba

  • Description modified (diff)

comment:3 Changed 2 years ago by mmezzarobba

  • Description modified (diff)

comment:4 Changed 2 years ago by git

  • Commit changed from 6323053dd30a3cf54fc99833a7a423c6472c14c8 to c3482e94fa911c1eb3c27c28ca76d2ad5df764cc

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2409b6f#28469 fix RBF(expr, rad)
329fe9dMerge branches '28469-arb-expr-rad', 'test_numerical_parent' and 'arb_gamma_zeta_w' into symbolic_functions_of_intervals
e88ac01sym/function: stricter notion of "numerical" arg
34e6521safer SR → RBF, SR → CBF conversions
c317f99test some more symbolic fun/interval combinations...
c3482e9warn about mixing SR with RIF/CIF

comment:5 Changed 2 years ago by git

  • Commit changed from c3482e94fa911c1eb3c27c28ca76d2ad5df764cc to e99589b8b1811eb299eaa6a2147c23b4ddf326fa

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d6afa4fpartially fix symbolic Γ, W, zetaderiv for ball arguments
f0cada3Merge branches '28469-arb-expr-rad', 'test_numerical_parent' and 'arb_gamma_zeta_w' into symbolic_functions_of_intervals
cfa1515sym/function: stricter notion of "numerical" arg
1b6f94dsafer SR → RBF, SR → CBF conversions
26bcfa3test some more symbolic fun/interval combinations...
a86b129warn about mixing SR with RIF/CIF
022771afunctions/error: remove erf(real_ball) hack
e99589bimplement conversions RIF, CIF → ZZ

comment:6 Changed 2 years ago by git

  • Commit changed from e99589b8b1811eb299eaa6a2147c23b4ddf326fa to cffa4c9cf50c082d5f26b460006bcfc7fc15422e

Branch pushed to git repo; I updated commit sha1. New commits:

cffa4c9improve SR → RBF, CBF conversions

comment:7 Changed 2 years ago by git

  • Commit changed from cffa4c9cf50c082d5f26b460006bcfc7fc15422e to 72b86c60c08f4832490a590383acf0df491185c4

Branch pushed to git repo; I updated commit sha1. New commits:

72b86c6#28517 minor doctest changes

comment:8 Changed 2 years ago by git

  • Commit changed from 72b86c60c08f4832490a590383acf0df491185c4 to 140363922f82fe4214e91f8699619f61c5d9fc4f

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7029baafunctions/error: remove erf(real_ball) hack
e29a8b4implement conversions RIF, CIF → ZZ
ddae661improve SR → RBF, CBF conversions
1403639#28517 minor doctest changes

comment:9 Changed 2 years ago by mmezzarobba

I'm done with the adjustments. The one test failure that the patchbot still reports does not seem linked to the changes made here.

comment:10 Changed 2 years ago by git

  • Commit changed from 140363922f82fe4214e91f8699619f61c5d9fc4f to 408b2d7483c4a5df4f48606c99b4ad3d8d9e2c74

Branch pushed to git repo; I updated commit sha1. New commits:

408b2d7#28517 also fix interval polylog()

comment:11 follow-up: Changed 2 years ago by vdelecroix

  • Status changed from needs_review to needs_work

Some tests are failing at 408b2d7

sage -t --long src/sage/functions/log.py
**********************************************************************
File "src/sage/functions/log.py", line 529, in sage.functions.log.Function_polylog.__init__
Failed example:
    polylog(2, BF(4/3))
Expected:
    [2.27001825336107090380391448586 +/- 5.64e-30] + [-0.90377988538400159956755721265 +/- 8.39e-30]*I
Got:
    nan
**********************************************************************
File "src/sage/functions/log.py", line 531, in sage.functions.log.Function_polylog.__init__
Failed example:
    parent(_)
Expected:
    Complex ball field with 100 bits of precision
Got:
    Real ball field with 100 bits of precision
**********************************************************************
1 item had failures:
   2 of  34 in sage.functions.log.Function_polylog.__init__
    [328 tests, 2 failures, 6.07 s]

comment:12 in reply to: ↑ 11 ; follow-up: Changed 2 years ago by mmezzarobba

Replying to vdelecroix:

Some tests are failing at 408b2d7

Thank you. I would say that returning NaN in this case is acceptable (and perhaps better than returning a complex interval--I'm not sure), so I'm leaning toward changing the test to pass a complex ball instead of a real one. What do you think?

comment:13 Changed 2 years ago by git

  • Commit changed from 408b2d7483c4a5df4f48606c99b4ad3d8d9e2c74 to 3afbb7b8898d2f570dd20604e789e5b1698adb03

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

47f06ccsafer SR → RBF, SR → CBF conversions
7b28050test some more symbolic fun/interval combinations...
0ae675dwarn about mixing SR with RIF/CIF
a0ef196functions/error: remove erf(real_ball) hack
8e97c48implement conversions RIF, CIF → ZZ
4cc4238improve SR → RBF, CBF conversions
1375464#28517 minor doctest changes
8b0a2b0#28517 also fix interval polylog()
a2ddc5d#28517 further extend SR to [RC]BF conversions
3afbb7b#28517 update doctest

comment:14 Changed 2 years ago by git

  • Commit changed from 3afbb7b8898d2f570dd20604e789e5b1698adb03 to 7f551dd55fb5eddcf7b7f530ce5649ce82ffe95e

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

9d09a00safer SR → RBF, SR → CBF conversions
c16b07ftest some more symbolic fun/interval combinations...
dda7073warn about mixing SR with RIF/CIF
e5a9f94functions/error: remove erf(real_ball) hack
f83fa29implement conversions RIF, CIF → ZZ
6f16987improve SR → RBF, CBF conversions
2ba0763#28517 minor doctest changes
1781e4e#28517 also fix interval polylog()
708cbfb#28517 further extend SR to [RC]BF conversions
7f551dd#28517 update doctest

comment:15 Changed 2 years ago by mmezzarobba

  • Status changed from needs_work to needs_review

rebased on py3-by-default beta

comment:16 Changed 2 years ago by git

  • Commit changed from 7f551dd55fb5eddcf7b7f530ce5649ce82ffe95e to 2d194119cc25703a1f542538a35b4f996b00dab5

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2d19411#28517 update doctest

comment:17 follow-up: Changed 2 years ago by vdelecroix

Why is there hasattr(parent, 'precision') in the method _is_numerical of symbolic elements?

comment:18 follow-up: Changed 2 years ago by vdelecroix

In the _is_numercal method of symbolics why not use directly the parent_is_numerical you introduced?

comment:19 in reply to: ↑ 17 Changed 2 years ago by mmezzarobba

Replying to vdelecroix:

Why is there hasattr(parent, 'precision') in the method _is_numerical of symbolic elements?

Part of the reason is that we don't want exact parents that correspond to real or complex numbers to be considered numerical in this context. I don't remember exactly why this is tested that way, but that predates this ticket.

comment:20 in reply to: ↑ 18 Changed 2 years ago by mmezzarobba

Replying to vdelecroix:

In the _is_numercal method of symbolics why not use directly the parent_is_numerical you introduced?

In part for the same reason as above. Probably also because I didn't want to deviate more than necessary from the behavior of the existing code in cases outside the scope of this ticket.

comment:21 in reply to: ↑ 12 ; follow-up: Changed 2 years ago by vdelecroix

Replying to mmezzarobba:

Replying to vdelecroix:

Some tests are failing at 408b2d7

Thank you. I would say that returning NaN in this case is acceptable (and perhaps better than returning a complex interval--I'm not sure), so I'm leaning toward changing the test to pass a complex ball instead of a real one. What do you think?

If nan is what it is supposed to be for real balls then this should still be tested. Similarly with sqrt(RBF(-2)), log(RBF(-2)), etc. Though many functions perform base extension silently

sage: sqrt(-2.0)
1.41421356237310*I
sage: log(-2.0)
0.693147180559945 + 3.14159265358979*I

Why RBF should be different?

comment:22 in reply to: ↑ 21 Changed 2 years ago by mmezzarobba

Replying to vdelecroix:

If nan is what it is supposed to be for real balls then this should still be tested. Similarly with sqrt(RBF(-2)), log(RBF(-2)), etc. Though many functions perform base extension silently

sage: sqrt(-2.0)
1.41421356237310*I
sage: log(-2.0)
0.693147180559945 + 3.14159265358979*I

Why RBF should be different?

I don't think it should (at least if the general rule for symbolic functions is to extend the domain, that is). As I remember it, there used to be a lot of inconsistencies regarding this point. But maybe I remember wrong, or things have improved.

Anyway, in the absence of any clear rule, I thought it safer not to do anything special for now. Changing from returning nan to returning a complex number later wouldn't break compatibility much compared to the inverse change. However, I'm completely open to having polylog(2, RBF(4/3)) return a complex number (preferably as part of another ticket focusing on consistency in that respect)

comment:23 Changed 2 years ago by mmezzarobba

  • Summary changed from Make the evaluation of symbolic functions at intervals more reliable to Make the evaluation of symbolic functions on intervals more reliable

On the off chance someone reading this has a bit of time to review it: this ticket fixes several subtly incorrect results that apparently already trapped real users, it would be nice if it could go in 9.0!

comment:24 Changed 2 years ago by git

  • Commit changed from 2d194119cc25703a1f542538a35b4f996b00dab5 to 34e20fa92263d1db0c99c7496cf33b72446c24d6

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

8bb3f0bsafer SR → RBF, SR → CBF conversions
0c99b47test some more symbolic fun/interval combinations...
d9fdcdcwarn about mixing SR with RIF/CIF
f410d33functions/error: remove erf(real_ball) hack
90ea363implement conversions RIF, CIF → ZZ
1189b69improve SR → RBF, CBF conversions
35eb85c#28517 minor doctest changes
19d255d#28517 also fix interval polylog()
b6045b6#28517 further extend SR to [RC]BF conversions
34e20fa#28517 update doctest

comment:25 Changed 2 years ago by embray

  • Milestone changed from sage-9.0 to sage-9.1

Ticket retargeted after milestone closed

comment:26 Changed 2 years ago by git

  • Commit changed from 34e20fa92263d1db0c99c7496cf33b72446c24d6 to d2ef7b00a726f40ede1e33ca3bc74de9664ec638

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

ee4ba9bsafer SR → RBF, SR → CBF conversions
af299f8test some more symbolic fun/interval combinations...
7b152e1warn about mixing SR with RIF/CIF
82032f8functions/error: remove erf(real_ball) hack
d81ede0implement conversions RIF, CIF → ZZ
c1baf1fimprove SR → RBF, CBF conversions
96d5f29#28517 minor doctest changes
72493a5#28517 also fix interval polylog()
7a0e3d6#28517 further extend SR to [RC]BF conversions
d2ef7b0#28517 update doctest

comment:27 Changed 2 years ago by git

  • Commit changed from d2ef7b00a726f40ede1e33ca3bc74de9664ec638 to baf74bbd4a632833774229e8166f5b3d1d07bd32

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

069cdecsafer SR → RBF, SR → CBF conversions
ebe5b7dtest some more symbolic fun/interval combinations...
88a988awarn about mixing SR with RIF/CIF
22341b1functions/error: remove erf(real_ball) hack
74bf17dimplement conversions RIF, CIF → ZZ
865e58eimprove SR → RBF, CBF conversions
ee87a29#28517 minor doctest changes
a9331d1#28517 also fix interval polylog()
b335df6#28517 further extend SR to [RC]BF conversions
baf74bb#28517 update doctest

comment:28 Changed 2 years ago by mmezzarobba

Rebased to fix a minor incompatibility with #26749.

comment:29 follow-up: Changed 22 months ago by vdelecroix

minor comments

  • In spite of its name -> Despite ...
  • In The following conversions used to yield incorrect enclosures:: mention the ticket number with :trac:`28517`.
  • importing inconditionnally from sage.symbolic.expression import Expression in the complex ball constructor is not a good idea (slowdown)

comment:30 in reply to: ↑ 29 Changed 22 months ago by mmezzarobba

Hi Vincent,

Thank you for your comments!

Replying to vdelecroix:

  • importing inconditionnally from sage.symbolic.expression import Expression in the complex ball constructor is not a good idea (slowdown)

Did you measure a slowdown (on what example) or you fear one? I'm asking because the import is not really unconditional: the fast path is the one where self.element_class(self, _x) succeeds.

comment:31 Changed 21 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.

comment:32 Changed 18 months ago by slabbe

  • Status changed from needs_review to needs_work

The branch needs to be rebased again.

comment:33 Changed 18 months ago by git

  • Commit changed from baf74bbd4a632833774229e8166f5b3d1d07bd32 to e7a49599cac8649068c0c966b427c621224f7a3c

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

026311atest some more symbolic fun/interval combinations...
79f5261warn about mixing SR with RIF/CIF
ea19845functions/error: remove erf(real_ball) hack
da19e0aimplement conversions RIF, CIF → ZZ
c35590bimprove SR → RBF, CBF conversions
2290c0a#28517 minor doctest changes
92aa61c#28517 also fix interval polylog()
b8846de#28517 further extend SR to [RC]BF conversions
cf805bf#28517 update doctest
e7a4959#28517 bug in real_nth_root() revealed by rebasing this ticket

comment:34 Changed 18 months ago by mmezzarobba

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:35 Changed 18 months ago by mmezzarobba

  • Description modified (diff)

comment:36 Changed 18 months ago by mkoeppe

  • Priority changed from major to critical
  • Reviewers set to Vincent Delecroix, Matthias Koeppe
  • Status changed from needs_review to positive_review

Looks like an important improvement, which we should get into 9.2.

comment:37 Changed 18 months ago by mmezzarobba

Thanks for the review!

comment:38 Changed 17 months ago by vbraun

  • Branch changed from u/mmezzarobba/symbolic_functions_of_intervals to e7a49599cac8649068c0c966b427c621224f7a3c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.