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:  sage9.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: 
Description (last modified by )
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, 1e10) sage: gamma(b, 1) [0.5072822338117733 +/ 1.35e17] sage: gamma(CBF(b), 1) [0.50728223381177 +/ 6.57e15] + [3.505711e11 +/ 2.19e18]*I sage: hurwitz_zeta(b, b) [1.948110822808643 +/ 2.28e16] sage: hurwitz_zeta(1/2, b) [2.019112205794726 +/ 5.00e16] sage: airy_ai(b) [0.07174949700810541 +/ 4.54e18] 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 bandaid 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
 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
comment:2 Changed 2 years ago by
 Description modified (diff)
comment:3 Changed 2 years ago by
 Description modified (diff)
comment:4 Changed 2 years ago by
 Commit changed from 6323053dd30a3cf54fc99833a7a423c6472c14c8 to c3482e94fa911c1eb3c27c28ca76d2ad5df764cc
comment:5 Changed 2 years ago by
 Commit changed from c3482e94fa911c1eb3c27c28ca76d2ad5df764cc to e99589b8b1811eb299eaa6a2147c23b4ddf326fa
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
d6afa4f  partially fix symbolic Γ, W, zetaderiv for ball arguments

f0cada3  Merge branches '28469arbexprrad', 'test_numerical_parent' and 'arb_gamma_zeta_w' into symbolic_functions_of_intervals

cfa1515  sym/function: stricter notion of "numerical" arg

1b6f94d  safer SR → RBF, SR → CBF conversions

26bcfa3  test some more symbolic fun/interval combinations...

a86b129  warn about mixing SR with RIF/CIF

022771a  functions/error: remove erf(real_ball) hack

e99589b  implement conversions RIF, CIF → ZZ

comment:6 Changed 2 years ago by
 Commit changed from e99589b8b1811eb299eaa6a2147c23b4ddf326fa to cffa4c9cf50c082d5f26b460006bcfc7fc15422e
Branch pushed to git repo; I updated commit sha1. New commits:
cffa4c9  improve SR → RBF, CBF conversions

comment:7 Changed 2 years ago by
 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
 Commit changed from 72b86c60c08f4832490a590383acf0df491185c4 to 140363922f82fe4214e91f8699619f61c5d9fc4f
comment:9 Changed 2 years ago by
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
 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 followup: ↓ 12 Changed 2 years ago by
 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.64e30] + [0.90377988538400159956755721265 +/ 8.39e30]*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 ; followup: ↓ 21 Changed 2 years ago by
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 intervalI'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
 Commit changed from 408b2d7483c4a5df4f48606c99b4ad3d8d9e2c74 to 3afbb7b8898d2f570dd20604e789e5b1698adb03
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
47f06cc  safer SR → RBF, SR → CBF conversions

7b28050  test some more symbolic fun/interval combinations...

0ae675d  warn about mixing SR with RIF/CIF

a0ef196  functions/error: remove erf(real_ball) hack

8e97c48  implement conversions RIF, CIF → ZZ

4cc4238  improve 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
 Commit changed from 3afbb7b8898d2f570dd20604e789e5b1698adb03 to 7f551dd55fb5eddcf7b7f530ce5649ce82ffe95e
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
9d09a00  safer SR → RBF, SR → CBF conversions

c16b07f  test some more symbolic fun/interval combinations...

dda7073  warn about mixing SR with RIF/CIF

e5a9f94  functions/error: remove erf(real_ball) hack

f83fa29  implement conversions RIF, CIF → ZZ

6f16987  improve 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
 Status changed from needs_work to needs_review
rebased on py3bydefault beta
comment:16 Changed 2 years ago by
 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 followup: ↓ 19 Changed 2 years ago by
Why is there hasattr(parent, 'precision')
in the method _is_numerical
of symbolic elements?
comment:18 followup: ↓ 20 Changed 2 years ago by
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
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
Replying to vdelecroix:
In the
_is_numercal
method of symbolics why not use directly theparent_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 ; followup: ↓ 22 Changed 2 years ago by
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 intervalI'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
Replying to vdelecroix:
If
nan
is what it is supposed to be for real balls then this should still be tested. Similarly withsqrt(RBF(2))
,log(RBF(2))
, etc. Though many functions perform base extension silentlysage: sqrt(2.0) 1.41421356237310*I sage: log(2.0) 0.693147180559945 + 3.14159265358979*IWhy
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
 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
 Commit changed from 2d194119cc25703a1f542538a35b4f996b00dab5 to 34e20fa92263d1db0c99c7496cf33b72446c24d6
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
8bb3f0b  safer SR → RBF, SR → CBF conversions

0c99b47  test some more symbolic fun/interval combinations...

d9fdcdc  warn about mixing SR with RIF/CIF

f410d33  functions/error: remove erf(real_ball) hack

90ea363  implement conversions RIF, CIF → ZZ

1189b69  improve 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
 Milestone changed from sage9.0 to sage9.1
Ticket retargeted after milestone closed
comment:26 Changed 2 years ago by
 Commit changed from 34e20fa92263d1db0c99c7496cf33b72446c24d6 to d2ef7b00a726f40ede1e33ca3bc74de9664ec638
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
ee4ba9b  safer SR → RBF, SR → CBF conversions

af299f8  test some more symbolic fun/interval combinations...

7b152e1  warn about mixing SR with RIF/CIF

82032f8  functions/error: remove erf(real_ball) hack

d81ede0  implement conversions RIF, CIF → ZZ

c1baf1f  improve 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
 Commit changed from d2ef7b00a726f40ede1e33ca3bc74de9664ec638 to baf74bbd4a632833774229e8166f5b3d1d07bd32
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
069cdec  safer SR → RBF, SR → CBF conversions

ebe5b7d  test some more symbolic fun/interval combinations...

88a988a  warn about mixing SR with RIF/CIF

22341b1  functions/error: remove erf(real_ball) hack

74bf17d  implement conversions RIF, CIF → ZZ

865e58e  improve 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
Rebased to fix a minor incompatibility with #26749.
comment:29 followup: ↓ 30 Changed 22 months ago by
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
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
 Milestone changed from sage9.1 to sage9.2
Moving tickets to milestone sage9.2 based on a review of last modification date, branch status, and severity.
comment:32 Changed 18 months ago by
 Status changed from needs_review to needs_work
The branch needs to be rebased again.
comment:33 Changed 18 months ago by
 Commit changed from baf74bbd4a632833774229e8166f5b3d1d07bd32 to e7a49599cac8649068c0c966b427c621224f7a3c
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
026311a  test some more symbolic fun/interval combinations...

79f5261  warn about mixing SR with RIF/CIF

ea19845  functions/error: remove erf(real_ball) hack

da19e0a  implement conversions RIF, CIF → ZZ

c35590b  improve 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
 Description modified (diff)
 Status changed from needs_work to needs_review
comment:35 Changed 18 months ago by
 Description modified (diff)
comment:36 Changed 18 months ago by
 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
Thanks for the review!
comment:38 Changed 17 months ago by
 Branch changed from u/mmezzarobba/symbolic_functions_of_intervals to e7a49599cac8649068c0c966b427c621224f7a3c
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
#28469 fix RBF(expr, rad)
Merge branches '28469arbexprrad', 'test_numerical_parent' and 'arb_gamma_zeta_w' into symbolic_functions_of_intervals
sym/function: stricter notion of "numerical" arg
safer SR → RBF, SR → CBF conversions
test some more symbolic fun/interval combinations...
warn about mixing SR with RIF/CIF