Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#24838 closed defect (fixed)

Upgrade to pynac-0.7.22

Reported by: Ralf Stephan Owned by:
Priority: major Milestone: sage-8.3
Component: symbolics Keywords:
Cc: François Bissey, Timo Kaufmann Merged in:
Authors: Ralf Stephan Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: e324b64 (Commits, GitHub, GitLab) Commit:
Dependencies: #24927 Stopgaps:

Status badges

Description (last modified by Ralf Stephan)

In the new version:

  • fast and working versons of quo_rem and rational decomposition (#25644, #25645)
  • fix power inconsistency
  • internal fix of another power inconsistency (#25639)
  • (x^a)^b --> x^(ab) IF x real, a odd integer, ab integer
  • add ex::treesize() as complexity metric (#25643)
  • information for packagers
  • implement commutative matching; fix GiNaC's matching deficiencies (#25168)
  • substitute patternless if no wildcards present
  • remove some compiler warnings
  • update AX_CXX_COMPILE_STDCXX.m4
  • fix exp(c*f(x)) simplification
  • fix cases(...).subs(...)
  • function::info for min/max_symbolic(#21945)
  • performance and readability improvements
  • correct sinh/cosh/tanh return type (#24299)
  • atan2 fixes
  • potential memleaks fixed
  • fixes in Giac interface
  • subs can now substitute numeric terms (#23964)
  • fix memleak in in-place PyObject numerics (#24745)
  • fix endless computation (#24883)
  • Python interface / Py3 fixes and improvements (thx Erik Bray, #24561, #24752, #24522)
  • fix internal asin/acos of complex balls
  • draw factors out of add^rational (#24768, #25251, #25252)
  • exp(f(x)), exp(c*f(x)), f inv. hyperbolic simplifications (#24841)
  • always extend trig/hyperbolic functions to complex domain (#24428)
  • extensive code readability improvements

https://github.com/pynac/pynac/releases/download/pynac-0.7.22/pynac-0.7.22.tar.bz2

Change History (75)

comment:1 Changed 5 years ago by Ralf Stephan

Branch: u/rws/upgrade_to_pynac_0_7_17

comment:2 Changed 5 years ago by Ralf Stephan

Authors: Ralf Stephan
Commit: d3511ce1cbd35b9d625b0f4215ee014e6cc99931
Description: modified (diff)
Status: newneeds_review

New commits:

e470685version / chkum / rm patch
d3511ce24838: doctest fixes

comment:3 Changed 5 years ago by François Bissey

Cc: François Bissey added

comment:4 Changed 5 years ago by Ralf Stephan

Description: modified (diff)

comment:5 Changed 5 years ago by Jeroen Demeyer

Just a question: is this supposed to fix #24522 too?

comment:6 Changed 5 years ago by Ralf Stephan

Description: modified (diff)

Yes, that PR is merged. I'll probably add a hotfix for #24883 here as patch, too.

comment:7 in reply to:  6 Changed 5 years ago by Jeroen Demeyer

Replying to rws:

I'll probably add a hotfix for #24883 here as patch, too.

Then I'm setting this to needs_work. If you change your mind, feel free to set this back to needs_review.

comment:8 Changed 5 years ago by Ralf Stephan

The issue is no longer there. Please review.

Last edited 5 years ago by Ralf Stephan (previous) (diff)

comment:9 Changed 5 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

This used to return a purely imaginary number, now there is a small real part:

sage: arccosh(x).subs(x=0.9)
-9.66146955461936e-22 + 0.451026811796262*I
Last edited 5 years ago by Jeroen Demeyer (previous) (diff)

comment:10 Changed 5 years ago by Ralf Stephan

This is an artifact of conversion to CC:

sage: CC(ComplexBallField(68)(0.9).arccosh())
-9.66146955461936e-22 + 0.451026811796262*I

We use arb with precision+15 and convert back to parent, all to make up for missing or limited (in the calculus sense, not the algebraic, note recent discussion) member functions of parent (RR in this case). Before this pynac version we had specific logic to zero the real part but this was error-prone. IMHO CC conversion of complex balls should be fixed.

comment:12 Changed 5 years ago by Ralf Stephan

You may also want to know why I'm using arb and not mpfr:

sage: r=srange(-2.5,2.5,.01)
sage: rr = [CC(i) for i in r]
sage: %timeit for x in rr: _=x.arccosh()
100 loops, best of 3: 11.9 ms per loop
sage: rr = [CBF(i) for i in r]
sage: %timeit for x in rr: _=x.arccosh()
1000 loops, best of 3: 649 µs per loop
Last edited 5 years ago by Ralf Stephan (previous) (diff)

comment:13 Changed 5 years ago by Ralf Stephan

What do you prefer, patching arb or the arb interface?

comment:15 in reply to:  12 Changed 5 years ago by Jeroen Demeyer

Replying to rws:

You may also want to know why I'm using arb and not mpfr:

Despite what you think, you are not using mpfr, but PARI:

    def arccosh(self):
        """
        Return the hyperbolic arccosine of ``self``.

        EXAMPLES::

            sage: (1+CC(I)).arccosh()
            1.06127506190504 + 0.904556894302381*I
        """
        return self._parent(self.__pari__().acosh())

If you want to compare with something, it should be mpc (in Sage: CC = MPComplexField(53))

comment:16 Changed 5 years ago by Ralf Stephan

Interestingly,

sage: arccosh(x).subs(x=MPComplexField(53)(0.9))
...
TypeError: no canonical coercion from Complex Field with 53 bits of precision to Symbolic Ring

comment:17 in reply to:  14 Changed 5 years ago by Ralf Stephan

Replying to rws:

See https://github.com/fredrik-johansson/arb/pull/210

This is now merged. Do we want that patch in this ticket? If not, anything else?

comment:18 Changed 5 years ago by Ralf Stephan

Status: needs_workneeds_review

comment:19 Changed 5 years ago by git

Commit: d3511ce1cbd35b9d625b0f4215ee014e6cc999319f86eb7b725b44939f22df4cb7f5a6b05935a6c8

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

e85fa6cMerge branch 'develop' into t/24838/upgrade_to_pynac_0_7_17
9f86eb724838: pynac-0.7.18 doctest fixes

comment:20 Changed 5 years ago by Ralf Stephan

Status: needs_reviewneeds_work

New release incoming.

comment:21 Changed 5 years ago by Ralf Stephan

Description: modified (diff)
Summary: Upgrade to pynac-0.7.17Upgrade to pynac-0.7.18

comment:22 Changed 5 years ago by git

Commit: 9f86eb7b725b44939f22df4cb7f5a6b05935a6c8ae5eac77131fca0266d63ee7c9b9f2b8ecf72ee8

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

ae5eac724838: version/chksum

comment:23 Changed 5 years ago by Ralf Stephan

Status: needs_workneeds_review

comment:24 Changed 5 years ago by Ralf Stephan

Description: modified (diff)

comment:25 Changed 5 years ago by Ralf Stephan

Status: needs_reviewneeds_work

0.7.17 introduced a bug in exp(ex*log(x)) simplification, and we need a new release, anyway.

comment:26 Changed 5 years ago by Ralf Stephan

Description: modified (diff)
Summary: Upgrade to pynac-0.7.18Upgrade to pynac-0.7.19

comment:27 Changed 5 years ago by git

Commit: ae5eac77131fca0266d63ee7c9b9f2b8ecf72ee888782837071d956743e49fb64ef972c1ee163db1

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

79d54eaMerge branch 'develop' into t/24838/upgrade_to_pynac_0_7_17
75b2f85fix doctests
887828324838: version/chksum

comment:28 Changed 5 years ago by Ralf Stephan

Status: needs_workneeds_review

comment:29 Changed 5 years ago by Jeroen Demeyer

This looks fishy:

-            sage: SR(CDF(1/2)).arccosh()  # rel tol 1e-15
+            sage: SR(CDF(1/2)).arccosh() # abs tol 1e-15

comment:30 Changed 5 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

The analogous thing as 9 should be fixed for pure imaginary results:

sage: CDF(1/2).arccosh()
1.0471975511965979*I
sage: SR(CDF(1/2)).arccosh()
-6.372599439108525e-21 + 1.0471975511965979*I

Then you also revert 29

comment:31 Changed 5 years ago by Ralf Stephan

Description: modified (diff)

comment:32 in reply to:  30 Changed 5 years ago by Ralf Stephan

Replying to jdemeyer:

The analogous thing as 9 should be fixed for pure imaginary results:

sage: CDF(1/2).arccosh()
1.0471975511965979*I
sage: SR(CDF(1/2)).arccosh()
-6.372599439108525e-21 + 1.0471975511965979*I

As explained in 10 missing FP functionality is gained using arb. But there was that bug in arb which is now fixed. Consequentially we need that fix or a recent arb to get the right result, see 14.

Then you also revert 29

Will re-revert.

comment:33 Changed 5 years ago by Ralf Stephan

Dependencies: #24927

comment:34 Changed 5 years ago by Ralf Stephan

I confirm that this works with https://github.com/fredrik-johansson/arb/commit/7afd3bfaf1697739751d96d3665ef83e78c76820 applied, which is in arb-2.13.0 (#24927).

comment:35 Changed 5 years ago by git

Commit: 88782837071d956743e49fb64ef972c1ee163db13efba1a6f9dd67401716dfdc56eb9bafa61e8963

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

8a2217aMerge branch 'develop' into t/24838/upgrade_to_pynac_0_7_17
3efba1afix doctest

comment:36 Changed 5 years ago by Ralf Stephan

Some recent Sage commit makes doctests (involving powers of I) fail together with pynac-0.7.17 so we'll need a fix in 0.7.20.

comment:37 Changed 5 years ago by Ralf Stephan

Actually it's not our fault: #25458.

comment:38 Changed 5 years ago by Ralf Stephan

Description: modified (diff)
Summary: Upgrade to pynac-0.7.19Upgrade to pynac-0.7.20

comment:39 Changed 5 years ago by git

Commit: 3efba1a6f9dd67401716dfdc56eb9bafa61e8963ad07cd5c0b8789b768d23b93db4a4004467ed5df

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

ed36070Merge branch 'develop' into t/24838/upgrade_to_pynac_0_7_17
3278103Merge branch 'develop' into t/24838/upgrade_to_pynac_0_7_17
ca69aefMerge branch 'develop' into t/24838/upgrade_to_pynac_0_7_17
97bf3b724838: doctest fixes
e6b01dfdoctest fix
fdb4210Merge branch 'develop' into t/24838/upgrade_to_pynac_0_7_17
ad07cd524838: 0.7.20 version/chksum

comment:40 Changed 4 years ago by Ralf Stephan

Branch: u/rws/upgrade_to_pynac_0_7_17
Commit: ad07cd5c0b8789b768d23b93db4a4004467ed5df
Description: modified (diff)
Milestone: sage-8.2sage-8.3
Summary: Upgrade to pynac-0.7.20Upgrade to pynac-0.7.22

comment:41 Changed 4 years ago by Ralf Stephan

Description: modified (diff)

comment:42 Changed 4 years ago by Ralf Stephan

Branch: u/rws/24838

comment:43 Changed 4 years ago by Ralf Stephan

Commit: 3b244e94f2b9bced8249d5abefb206bd419f8f43
Status: needs_workneeds_review

The dependency is now in. Here a fresh branch with the newest Pynac.


New commits:

5f6c08e24838: pkg version / chksum / remove old patch
3b244e924838: doctest fixes

comment:44 Changed 4 years ago by Ralf Stephan

Description: modified (diff)

comment:45 Changed 4 years ago by Frédéric Chapoton

I got "invalid checksum"

comment:46 Changed 4 years ago by Ralf Stephan

Please check again, I just now did not get any error with the file linked in the ticket. Did you reload the branch?

comment:47 Changed 4 years ago by Frédéric Chapoton

Indeed. Works now. I must have made some mistake in my previous attempt.

comment:48 Changed 4 years ago by Frédéric Chapoton

got

sage -t --long src/sage/rings/asymptotic/asymptotics_multivariate_generating_functions.py  # 2 doctests failed
sage -t --long src/sage/symbolic/expression.pyx  # 1 doctest failed

comment:49 Changed 4 years ago by Frédéric Chapoton

Doctesting 2 files.
sage -t --long src/sage/rings/asymptotic/asymptotics_multivariate_generating_functions.py
**********************************************************************
File "src/sage/rings/asymptotic/asymptotics_multivariate_generating_functions.py", line 153, in sage.rings.asymptotic.asymptotics_multivariate_generating_functions
Failed example:
    F1.asymptotics(p, alpha, 2)
Expected:
    (-3*((2*a^2 - 5*a*b + 2*b^2)*r^2 + (a + b)*r + 3)*((1/3)^(-a)*(1/3)^(-b))^r,
     (1/3)^(-a)*(1/3)^(-b), -3*(2*a^2 - 5*a*b + 2*b^2)*r^2 - 3*(a + b)*r - 9)
Got:
    (-3*((2*a^2 - 5*a*b + 2*b^2)*r^2 + (a + b)*r + 3)*(1/((1/3)^a*(1/3)^b))^r,
     1/((1/3)^a*(1/3)^b),
     -3*(2*a^2 - 5*a*b + 2*b^2)*r^2 - 3*(a + b)*r - 9)
**********************************************************************
File "src/sage/rings/asymptotic/asymptotics_multivariate_generating_functions.py", line 2161, in sage.rings.asymptotic.asymptotics_multivariate_generating_functions.FractionWithFactoredDenominator.?
Failed example:
    F.asymptotics_multiple(p, alpha, 2, var('r')) # long time
Expected:
    (3*((1/3)^(-a)*(1/3)^(-b))^r*e^(2/3), (1/3)^(-a)*(1/3)^(-b), 3*e^(2/3))
Got:
    (3*(1/((1/3)^a*(1/3)^b))^r*e^(2/3), 1/((1/3)^a*(1/3)^b), 3*e^(2/3))
**********************************************************************
2 items had failures:
   1 of  77 in sage.rings.asymptotic.asymptotics_multivariate_generating_functions

and

Failed example:
    SR(CDF(1/2)).arccosh() #  rel tol 1e-15
Expected:
    1.0471975511965976*I
Got:
    -6.372599439108525e-21 + 1.0471975511965979*I

comment:50 in reply to:  49 Changed 4 years ago by Ralf Stephan

Replying to chapoton:

Failed example:
    SR(CDF(1/2)).arccosh() #  rel tol 1e-15
Expected:
    1.0471975511965976*I
Got:
    -6.372599439108525e-21 + 1.0471975511965979*I

Not confirmed. You apparently don't have the dependency installed, the newest arb.

comment:51 Changed 4 years ago by git

Commit: 3b244e94f2b9bced8249d5abefb206bd419f8f43814e54994ad4ef7fa456638bf7c7e6c82aad347f

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

814e54924838: fix to doctests

comment:52 Changed 4 years ago by Frédéric Chapoton

The second doctest is still failing for me.

sage: version()
'SageMath version 8.3.beta7, Release Date: 2018-06-23'
sage: package_versions('standard')['arb']
('2.13.0', '2.13.0')

comment:53 in reply to:  52 Changed 4 years ago by Ralf Stephan

Replying to chapoton:

The second doctest is still failing for me.

Can you please give the output of:

sage: CDF(1/2).arccosh()
1.0471975511965979*I
sage: CBF(1/2).arccosh()
[1.047197551196598 +/- 7.94e-16]*I

comment:54 Changed 4 years ago by Frédéric Chapoton

sage: CDF(1/2).arccosh()
1.0471975511965979*I
sage: CBF(1/2).arccosh()
[+/- 4.90e-16] + [1.047197551196598 +/- 7.94e-16]*I

comment:55 Changed 4 years ago by Ralf Stephan

This is peculiar. I was under the impression that this fix: https://github.com/fredrik-johansson/arb/commit/7afd3bfaf1697739751d96d3665ef83e78c76820 would be in arb-2.13.0. I just found out that it was committed Mar 5, while the release was Feb 23. I should have actively propagated that patch into our arb upgrade ticket.

Should I open a separate ticket for the arb patch or just put it into this branch?

I would have noticed earlier but for some reason I don't get the error with 2.13.0.

comment:56 Changed 4 years ago by Ralf Stephan

Ah, I had the patch still in the patches directory. New files are not deleted when changing branches.

comment:57 Changed 4 years ago by git

Commit: 814e54994ad4ef7fa456638bf7c7e6c82aad347f6645f7923ee5bdd97219b0a475e3ae98e03de4e8

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

6645f7924838: add arb commit 7afd3afbf

comment:58 Changed 4 years ago by Ralf Stephan

comment:59 Changed 4 years ago by Frédéric Chapoton

This should bump the arb version by .p0

comment:60 Changed 4 years ago by git

Commit: 6645f7923ee5bdd97219b0a475e3ae98e03de4e88e4e081d09ff059a8f192ec57ba0443587da3900

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

8e4e08124838: bump arb version

comment:61 Changed 4 years ago by Frédéric Chapoton

Thanks. This should be ready to go.

Launching now "make ptestlong" once again, just to be sure.

comment:62 Changed 4 years ago by Frédéric Chapoton

Reviewers: Frédéric Chapoton
Status: needs_reviewpositive_review

ok, all tests pass

comment:63 Changed 4 years ago by Ralf Stephan

Thanks for the review!

comment:64 Changed 4 years ago by Volker Braun

Status: positive_reviewneeds_work

On 32-bit:

File "src/sage/functions/hypergeometric.py", line 148, in sage.functions.hypergeometric
Failed example:
    hypergeometric_U(2, 2, x).series(x == 3, 100).subs(x=1).n()
Expected:
    0.403652637676806
Got:
    0.403651580752398

comment:65 in reply to:  64 ; Changed 4 years ago by Ralf Stephan

Replying to vbraun:

On 32-bit:

File "src/sage/functions/hypergeometric.py", line 148, in sage.functions.hypergeometric
Failed example:
    hypergeometric_U(2, 2, x).series(x == 3, 100).subs(x=1).n()
Expected:
    0.403652637676806
Got:
    0.403651580752398

Pynac is not involved in the numerics (hypergeometric_U is not a GinacFunction), so the hypothesis is that the expression before .n() is different with 32bit pynac-0.7.22.

comment:66 Changed 4 years ago by Timo Kaufmann

Cc: Timo Kaufmann added

comment:67 in reply to:  65 Changed 4 years ago by Ralf Stephan

Replying to rws:

Replying to vbraun:

On 32-bit:

Pynac is not involved in the numerics (hypergeometric_U is not a GinacFunction), so the hypothesis is that the expression before .n() is different with 32bit pynac-0.7.22.

We will never know. I failed to install 32bit versions of CentOS and ArchLinux in a VM, and I will not try again. There will be a ticket for this doctest failure, and the test marked "known bug (see ...".

comment:68 Changed 4 years ago by git

Commit: 8e4e081d09ff059a8f192ec57ba0443587da3900e324b642c1f7c7cb98eea400abfe7c9a4296fcca

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

5f6c08e24838: pkg version / chksum / remove old patch
3b244e924838: doctest fixes
814e54924838: fix to doctests
6645f7924838: add arb commit 7afd3afbf
8e4e08124838: bump arb version
e324b6424838: mark 32-bit fail as known bug, see 25688

comment:69 Changed 4 years ago by Ralf Stephan

Status: needs_workpositive_review

comment:70 Changed 4 years ago by Timo Kaufmann

This isn't really about this specific ticket (thanks for putting in the work for the upgrade), but is there a reason everybody seems to make seperate "pkg version / chksum" and "doctest fixes" patches?

Shouldn't each commit be one unit of changes for which the doctests still pass?

comment:71 in reply to:  70 ; Changed 4 years ago by Ralf Stephan

Replying to gh-timokau:

Shouldn't each commit be one unit of changes for which the doctests still pass?

The only reason I see for this is to make statistics based on number of commits meaningful. By splitting commits into parts (if there is such a natural split, maybe here it's less needed) I make the job of reviewing easier.

comment:72 in reply to:  71 Changed 4 years ago by Timo Kaufmann

Replying to rws:

Replying to gh-timokau:

Shouldn't each commit be one unit of changes for which the doctests still pass?

The only reason I see for this is to make statistics based on number of commits meaningful. By splitting commits into parts (if there is such a natural split, maybe here it's less needed) I make the job of reviewing easier.

A much more practical reason is the use of git-bisect or just generally being able to check out some interesting commit and build from it.

comment:73 Changed 4 years ago by Volker Braun

Branch: u/rws/24838e324b642c1f7c7cb98eea400abfe7c9a4296fcca
Resolution: fixed
Status: positive_reviewclosed

comment:74 in reply to:  57 ; Changed 4 years ago by Timo Kaufmann

Commit: e324b642c1f7c7cb98eea400abfe7c9a4296fcca

Replying to git:

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

6645f7924838: add arb commit 7afd3afbf

For the future it would greatly simplify packaging if you would add some documentation when adding a patch. A link to the github issue/pr and maybe a sentence about what it does would be sufficient. (The commit is actually 7afd3bfaf)

comment:75 in reply to:  74 Changed 4 years ago by Jeroen Demeyer

Replying to gh-timokau:

For the future it would greatly simplify packaging if you would add some documentation when adding a patch. A link to the github issue/pr and maybe a sentence about what it does would be sufficient. (The commit is actually 7afd3bfaf)

+1

Patches without any documentation or links should be rejected.

Note: See TracTickets for help on using tickets.