Opened 3 years ago
Closed 2 years ago
#24927 closed enhancement (fixed)
Upgrade arb to 2.13.0
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage8.3 
Component:  packages: standard  Keywords:  
Cc:  mmezzarobba, fredrik.johansson, fbissey  Merged in:  
Authors:  Ralf Stephan, Frédéric Chapoton, Marc Mezzarobba  Reviewers:  Ralf Stephan 
Report Upstream:  N/A  Work issues:  
Branch:  e0e3c06 (Commits)  Commit:  e0e3c06f561515d83b0c8803639f26efa4f5bf11 
Dependencies:  Stopgaps: 
Description (last modified by )
Upstream at
https://github.com/fredrikjohansson/arb/archive/2.13.0.tar.gz
(to be renamed arb2.13.0.tar.gz
)
Many doctests have to be changed because 1/0
now returns nan
rather than [+/ inf]
. See also the announcement on flintdevel.
Issues about enclosure regressions reported upstream at Issue #227.
Change History (37)
comment:1 Changed 3 years ago by
 Cc fbissey added
comment:2 Changed 3 years ago by
 Dependencies set to #24575
comment:3 Changed 3 years ago by
 Description modified (diff)
comment:4 Changed 3 years ago by
comment:5 followup: ↓ 7 Changed 3 years ago by
It may be better to wait for the existing arbrelated tickets to be merged, in case the upgrade causes doctest failures there...
comment:6 followup: ↓ 29 Changed 3 years ago by
And this is annoying
sage: RBF([infinity,infinity]) nan
comment:7 in reply to: ↑ 5 ; followup: ↓ 8 Changed 3 years ago by
Replying to mmezzarobba:
It may be better to wait for the existing arbrelated tickets to be merged, in case the upgrade causes doctest failures there...
Could you add them as dependencies?
comment:8 in reply to: ↑ 7 Changed 3 years ago by
Replying to vdelecroix:
Replying to mmezzarobba:
It may be better to wait for the existing arbrelated tickets to be merged, in case the upgrade causes doctest failures there...
Could you add them as dependencies?
Yes, but I also wouldn't want to block this ticket if nobody is interested in reviewing the other ones. In any case, the tickets I have in mind are #24314, #24816, #24717, #24626.
comment:9 Changed 3 years ago by
 Dependencies changed from #24575 to #24575, #24314, #24816, #24717, #24626
comment:10 Changed 3 years ago by
Coming late to the party, pynac0.7.17 (#24838) depends on this because a fix is included.
comment:11 followup: ↓ 12 Changed 3 years ago by
Why is #24575 a dependency? That seems to have nothing to do with arb.
comment:12 in reply to: ↑ 11 Changed 3 years ago by
 Dependencies changed from #24575, #24314, #24816, #24717, #24626 to #24314, #24816, #24717, #24626
comment:13 Changed 3 years ago by
 Description modified (diff)
 Milestone changed from sage8.2 to sage8.3
comment:14 Changed 3 years ago by
Dependencies are merged now. Anyone working on this?
comment:15 Changed 3 years ago by
 Branch set to u/rws/upgrade_arb_to_2_13_0
comment:16 Changed 3 years ago by
 Commit set to 9c8b1c7250b7a9cd2cca0660d3b5ee38f53bad89
sage t warnlong 52.8 src/sage/rings/complex_arb.pyx # 35 doctests failed sage t warnlong 52.8 src/sage/rings/real_arb.pyx # 10 doctests failed sage t warnlong 52.8 src/sage/rings/polynomial/polynomial_complex_arb.pyx # 2 doctests failed
Please someone go ahead. I can't work on this until end of June.
New commits:
9c8b1c7  24927: version/chksum/rm patch

comment:17 Changed 2 years ago by
 Branch changed from u/rws/upgrade_arb_to_2_13_0 to public/24927
 Commit changed from 9c8b1c7250b7a9cd2cca0660d3b5ee38f53bad89 to 0845bcdc6125ea14d1fc05e6e21a2a2b02949f63
 Dependencies #24314, #24816, #24717, #24626 deleted
comment:18 Changed 2 years ago by
 Commit changed from 0845bcdc6125ea14d1fc05e6e21a2a2b02949f63 to 50ac79a14624a50103379478e9fcfb214710596f
Branch pushed to git repo; I updated commit sha1. New commits:
50ac79a  try to fix arb CBF doctests

comment:19 followup: ↓ 20 Changed 2 years ago by
One of the remaining doctest failures come from the following.
There is a regression, before:
sage: sin(1/RBF(0))*0 0
after:
sage: sin(1/RBF(0))*0 nan
comment:20 in reply to: ↑ 19 Changed 2 years ago by
Replying to chapoton:
sage: sin(1/RBF(0))*0 nan
That's because of
sage: 1/RBF(0) nan
which was infinite before.
comment:21 followups: ↓ 22 ↓ 26 Changed 2 years ago by
Indeed. But what is the semantic of "nan" ? Should we expect sin(nan)
to be [+/ 1]
?
comment:22 in reply to: ↑ 21 Changed 2 years ago by
Replying to chapoton:
Indeed. But what is the semantic of "nan" ?
Should we expect
sin(nan)
to be[+/ 1]
?
No. And I think that's more or less the point of the change: sin(1/x) ∈ [1,1] for all real x close to zero, but returning a finite ball could give the impression that sin(1/x) is analytic at zero (since, for complex balls, finite result ⇒ analytic).
In any case, the resulting enclosure is correct (if coarser than before), and if upstream decided to make that change, I think we should follow along.
comment:23 Changed 2 years ago by
 Commit changed from 50ac79a14624a50103379478e9fcfb214710596f to 2662caf487050118cd300fb6b8c5c17453bc0e36
Branch pushed to git repo; I updated commit sha1. New commits:
2662caf  fixing doctests of oscillating integral of x sin(1/x)

comment:24 Changed 2 years ago by
 Commit changed from 2662caf487050118cd300fb6b8c5c17453bc0e36 to ba31bd51a1338dc2938a8d60d95fb6d10a7acee3
Branch pushed to git repo; I updated commit sha1. New commits:
ba31bd5  fix doctests in real_arb by removing extended_line

comment:25 Changed 2 years ago by
 Status changed from new to needs_info
I may have made some controversial changes.. please tell me.
comment:26 in reply to: ↑ 21 Changed 2 years ago by
Replying to chapoton:
Indeed. But what is the semantic of "nan" ? Should we expect
sin(nan)
to be[+/ 1]
?
FWIW, nan
/NaN
always leads to collapse of the expression tree.
comment:27 Changed 2 years ago by
 Commit changed from ba31bd51a1338dc2938a8d60d95fb6d10a7acee3 to 33186762d670825068722a44c6212002f98a9ccd
comment:28 Changed 2 years ago by
 Commit changed from 33186762d670825068722a44c6212002f98a9ccd to b29c09685f032ac2874f083f4fe70516464f5be7
Branch pushed to git repo; I updated commit sha1. New commits:
b29c096  #24927 remove old workaround, add a new one...

comment:29 in reply to: ↑ 6 Changed 2 years ago by
 Status changed from needs_info to needs_review
Replying to vdelecroix:
And this is annoying
sage: RBF([infinity,infinity]) nan
I don't think this is a new issue, but I took the occasion to fix it.
comment:30 Changed 2 years ago by
make ptestlong
gives me two failures:
File "src/sage/functions/other.py", line 1418, in sage.functions.other.Function_factorial._eval_ Failed example: factorial(RBF(2)**64) Expected: [+/ 2.30e+347382171305201370464] Got: [+/ 2.30e+347382171326740403407] File "src/sage/matrix/matrix_complex_ball_dense.pyx", line 637, in sage.matrix.matrix_complex_ball_dense.Matrix_complex_ball_dense.charpoly Failed example: mat.charpoly(algorithm="hessenberg") Expected: x^5 + ([1.8 +/ 0.0445])*x^4 + ([0.3 +/ 0.0833])*x^3 + ([+/ 0.0164])*x^2 + ([+/ 6.01e4])*x + [+/ 6.97e6] Got: x^5 + ([1.8 +/ 0.0445])*x^4 + ([0.3 +/ 0.0828])*x^3 + ([+/ 0.0163])*x^2 + ([+/ 5.95e4])*x + [+/ 6.83e6]
In the first one, using Stirling's formula confirms the regression:
sage: (log(sqrt(2*pi*2^64),10) + log(2^64/e,10)*2^64).n() 3.47382171305201e20
comment:31 followup: ↓ 32 Changed 2 years ago by
 Reviewers set to Ralf Stephan
If you mark the first test above "known bug", and fix the second then it's positive review from me.
comment:32 in reply to: ↑ 31 ; followup: ↓ 35 Changed 2 years ago by
Replying to rws:
If you mark the first test above "known bug", and fix the second then it's positive review from me.
It is not a bug, even if the enclosure is a bit less tight as it used to... I think we should just change the expected output.
comment:33 Changed 2 years ago by
 Commit changed from b29c09685f032ac2874f083f4fe70516464f5be7 to bebabb48df4c288e4874aac3f3aa6cec77265d08
Branch pushed to git repo; I updated commit sha1. New commits:
bebabb4  trac 24927 fixing 2 doctests (one is tagged known bug)

comment:34 Changed 2 years ago by
 Commit changed from bebabb48df4c288e4874aac3f3aa6cec77265d08 to e0e3c06f561515d83b0c8803639f26efa4f5bf11
Branch pushed to git repo; I updated commit sha1. New commits:
e0e3c06  change one doctest result

comment:35 in reply to: ↑ 32 Changed 2 years ago by
Replying to mmezzarobba:
Replying to rws:
If you mark the first test above "known bug", and fix the second then it's positive review from me.
It is not a bug, even if the enclosure is a bit less tight as it used to... I think we should just change the expected output.
ok, I just did that
comment:37 Changed 2 years ago by
 Branch changed from public/24927 to e0e3c06f561515d83b0c8803639f26efa4f5bf11
 Resolution set to fixed
 Status changed from positive_review to closed
Among the changed behavior some of them are weird (and perhaps not desirable)
I also noted some small regressions in enclosures (ignoring the ones that get better)