Opened 22 months ago

Closed 17 months ago

Last modified 17 months ago

#24838 closed defect (fixed)

Upgrade to pynac-0.7.22

Reported by: rws Owned by:
Priority: major Milestone: sage-8.3
Component: symbolics Keywords:
Cc: fbissey, gh-timokau Merged in:
Authors: Ralf Stephan Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: e324b64 (Commits) Commit:
Dependencies: #24927 Stopgaps:

Description (last modified by rws)

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 22 months ago by rws

  • Branch set to u/rws/upgrade_to_pynac_0_7_17

comment:2 Changed 22 months ago by rws

  • Authors set to Ralf Stephan
  • Commit set to d3511ce1cbd35b9d625b0f4215ee014e6cc99931
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

e470685version / chkum / rm patch
d3511ce24838: doctest fixes

comment:3 Changed 22 months ago by fbissey

  • Cc fbissey added

comment:4 Changed 22 months ago by rws

  • Description modified (diff)

comment:5 Changed 21 months ago by jdemeyer

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

comment:6 follow-up: Changed 21 months ago by rws

  • 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 21 months ago by jdemeyer

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 21 months ago by rws

The issue is no longer there. Please review.

Last edited 21 months ago by rws (previous) (diff)

comment:9 Changed 21 months ago by jdemeyer

  • Status changed from needs_review to needs_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 21 months ago by jdemeyer (previous) (diff)

comment:10 Changed 21 months ago by rws

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 follow-up: Changed 21 months ago by rws

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 21 months ago by rws (previous) (diff)

comment:13 Changed 21 months ago by rws

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

comment:15 in reply to: ↑ 12 Changed 21 months ago by jdemeyer

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 21 months ago by rws

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 21 months ago by rws

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 21 months ago by rws

  • Status changed from needs_work to needs_review

comment:19 Changed 21 months ago by git

  • Commit changed from d3511ce1cbd35b9d625b0f4215ee014e6cc99931 to 9f86eb7b725b44939f22df4cb7f5a6b05935a6c8

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 21 months ago by rws

  • Status changed from needs_review to needs_work

New release incoming.

comment:21 Changed 21 months ago by rws

  • Description modified (diff)
  • Summary changed from Upgrade to pynac-0.7.17 to Upgrade to pynac-0.7.18

comment:22 Changed 21 months ago by git

  • Commit changed from 9f86eb7b725b44939f22df4cb7f5a6b05935a6c8 to ae5eac77131fca0266d63ee7c9b9f2b8ecf72ee8

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

ae5eac724838: version/chksum

comment:23 Changed 21 months ago by rws

  • Status changed from needs_work to needs_review

comment:24 Changed 21 months ago by rws

  • Description modified (diff)

comment:25 Changed 20 months ago by rws

  • Status changed from needs_review to needs_work

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

comment:26 Changed 20 months ago by rws

  • Description modified (diff)
  • Summary changed from Upgrade to pynac-0.7.18 to Upgrade to pynac-0.7.19

comment:27 Changed 20 months ago by git

  • Commit changed from ae5eac77131fca0266d63ee7c9b9f2b8ecf72ee8 to 88782837071d956743e49fb64ef972c1ee163db1

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 20 months ago by rws

  • Status changed from needs_work to needs_review

comment:29 Changed 20 months ago by jdemeyer

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 follow-up: Changed 20 months ago by jdemeyer

  • Status changed from needs_review to needs_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 20 months ago by rws

  • Description modified (diff)

comment:32 in reply to: ↑ 30 Changed 20 months ago by rws

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 20 months ago by rws

  • Dependencies set to #24927

comment:34 Changed 20 months ago by rws

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 20 months ago by git

  • Commit changed from 88782837071d956743e49fb64ef972c1ee163db1 to 3efba1a6f9dd67401716dfdc56eb9bafa61e8963

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 19 months ago by rws

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 19 months ago by rws

Actually it's not our fault: #25458.

comment:38 Changed 19 months ago by rws

  • Description modified (diff)
  • Summary changed from Upgrade to pynac-0.7.19 to Upgrade to pynac-0.7.20

comment:39 Changed 19 months ago by git

  • Commit changed from 3efba1a6f9dd67401716dfdc56eb9bafa61e8963 to ad07cd5c0b8789b768d23b93db4a4004467ed5df

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 18 months ago by rws

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

comment:41 Changed 18 months ago by rws

  • Description modified (diff)

comment:42 Changed 18 months ago by rws

  • Branch set to u/rws/24838

comment:43 Changed 18 months ago by rws

  • Commit set to 3b244e94f2b9bced8249d5abefb206bd419f8f43
  • Status changed from needs_work to needs_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 18 months ago by rws

  • Description modified (diff)

comment:45 Changed 18 months ago by chapoton

I got "invalid checksum"

comment:46 Changed 18 months ago by rws

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 18 months ago by chapoton

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

comment:48 Changed 18 months ago by 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 follow-up: Changed 18 months ago by 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 18 months ago by rws

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 18 months ago by git

  • Commit changed from 3b244e94f2b9bced8249d5abefb206bd419f8f43 to 814e54994ad4ef7fa456638bf7c7e6c82aad347f

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

814e54924838: fix to doctests

comment:52 follow-up: Changed 18 months ago by 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 18 months ago by rws

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 18 months ago by 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 18 months ago by rws

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 18 months ago by rws

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

comment:57 follow-up: Changed 18 months ago by git

  • Commit changed from 814e54994ad4ef7fa456638bf7c7e6c82aad347f to 6645f7923ee5bdd97219b0a475e3ae98e03de4e8

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

6645f7924838: add arb commit 7afd3afbf

comment:58 Changed 18 months ago by rws

comment:59 Changed 18 months ago by chapoton

This should bump the arb version by .p0

comment:60 Changed 18 months ago by git

  • Commit changed from 6645f7923ee5bdd97219b0a475e3ae98e03de4e8 to 8e4e081d09ff059a8f192ec57ba0443587da3900

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

8e4e08124838: bump arb version

comment:61 Changed 18 months ago by chapoton

Thanks. This should be ready to go.

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

comment:62 Changed 18 months ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, all tests pass

comment:63 Changed 18 months ago by rws

Thanks for the review!

comment:64 follow-up: Changed 18 months ago by vbraun

  • Status changed from positive_review to needs_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 ; follow-up: Changed 18 months ago by rws

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 18 months ago by gh-timokau

  • Cc gh-timokau added

comment:67 in reply to: ↑ 65 Changed 18 months ago by rws

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 18 months ago by git

  • Commit changed from 8e4e081d09ff059a8f192ec57ba0443587da3900 to e324b642c1f7c7cb98eea400abfe7c9a4296fcca

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 18 months ago by rws

  • Status changed from needs_work to positive_review

comment:70 follow-up: Changed 18 months ago by gh-timokau

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 ; follow-up: Changed 18 months ago by 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.

comment:72 in reply to: ↑ 71 Changed 18 months ago by gh-timokau

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 17 months ago by vbraun

  • Branch changed from u/rws/24838 to e324b642c1f7c7cb98eea400abfe7c9a4296fcca
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:74 in reply to: ↑ 57 ; follow-up: Changed 17 months ago by gh-timokau

  • Commit e324b642c1f7c7cb98eea400abfe7c9a4296fcca deleted

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 17 months ago by jdemeyer

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.