Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#25567 closed enhancement (fixed)

Upgrade to pari-2.11

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.3
Component: packages: standard Keywords:
Cc: fbissey, cremona, gh-timokau, frederichan Merged in:
Authors: Jeroen Demeyer Reviewers: Timo Kaufmann
Report Upstream: Fixed upstream, but not in a stable release. Work issues:
Branch: 739f7a6 (Commits) Commit:
Dependencies: #26010 Stopgaps:

Attachments (1)

pols.sage (2.8 KB) - added by cremona 2 years ago.
Sage input file which causes a pari crash with current Sage/Pari? versino

Download all attachments as: .zip

Change History (83)

comment:1 Changed 2 years ago by fbissey

  • Cc fbissey added

comment:2 Changed 2 years ago by jdemeyer

  • Cc cremona added

comment:3 Changed 2 years ago by jdemeyer

  • Branch set to u/jdemeyer/upgrade_to_pari_2_10_0_alpha

Changed 2 years ago by cremona

Sage input file which causes a pari crash with current Sage/Pari? versino

comment:4 follow-up: Changed 2 years ago by cremona

  • Commit set to 9ed413d71d814afe11255c6e77c319d124a3bec6

See the attached file which crashes using 8.1's pari version. Testing suggests that this will be OK in 2.10.0.alpha, and this should be verified before this ticket is merged.


New commits:

9ed413dUpgrade to pari-2.10.0.alpha

comment:5 Changed 2 years ago by git

  • Commit changed from 9ed413d71d814afe11255c6e77c319d124a3bec6 to 68662b18cf4f0e5540a0dedddfb3539cacc6ac70

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

68662b1Fix doctests for PARI/GP upgrade

comment:6 in reply to: ↑ 4 ; follow-up: Changed 2 years ago by jdemeyer

Replying to cremona:

See the attached file which crashes using 8.1's pari version. Testing suggests that this will be OK in 2.10.0.alpha, and this should be verified before this ticket is merged.

I'm not sure whether you would propose to add it as doctest, but I'd rather not since it takes quite a long time.

comment:7 in reply to: ↑ 6 Changed 2 years ago by cremona

Replying to jdemeyer:

Replying to cremona:

See the attached file which crashes using 8.1's pari version. Testing suggests that this will be OK in 2.10.0.alpha, and this should be verified before this ticket is merged.

I'm not sure whether you would propose to add it as doctest, but I'd rather not since it takes quite a long time.

No, I agree. But at least we could have an assertion on this ticket that it works, so that whoever reviews the ticket (or me) can test it manually.

comment:8 Changed 2 years ago by jdemeyer

  • Description modified (diff)
  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

Thanks to a Sage doctest, I found a genuine bug in the new PARI. So we'll need to wait for this to be fixed before we can proceed.

comment:9 follow-up: Changed 2 years ago by cremona

I could not find pari bug 2055, but 2054 is possibly similar?

comment:10 in reply to: ↑ 9 Changed 2 years ago by jdemeyer

Replying to cremona:

I could not find pari bug 2055

It should be there now. Their bug tracker sometimes takes a few minutes to update.

comment:11 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:12 Changed 2 years ago by git

  • Commit changed from 68662b18cf4f0e5540a0dedddfb3539cacc6ac70 to e8cd3ab7942184ad9541ac860a7a446b9407d0f0

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

b6cf1e7Upgrade to pari-2.10.0.alpha
e8cd3abFix doctests for PARI/GP upgrade

comment:13 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:14 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:15 Changed 2 years ago by git

  • Commit changed from e8cd3ab7942184ad9541ac860a7a446b9407d0f0 to cceabac9faf935dd21cad0c9a55160ee90039c7b

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

a9f42a2Upgrade to pari-2.10.0.alpha
cceabacFix doctests for PARI/GP upgrade

comment:16 Changed 2 years ago by jdemeyer

So I ran all doctests and found 3 independent new bugs in PARI. One was already fixed in master, I reported two and one of those is already fixed. So this ticket is essentially "needs review" modulo the elleta issue.

comment:17 Changed 2 years ago by git

  • Commit changed from cceabac9faf935dd21cad0c9a55160ee90039c7b to a62587a08668adf537fb3fcbb418b39dd8508b1f

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

72d7ba3Upgrade to pari-2.10.0.alpha
a62587aFix doctests for PARI/GP upgrade

comment:18 Changed 2 years ago by jdemeyer

  • Description modified (diff)
  • Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.
  • Status changed from new to needs_review

comment:19 Changed 2 years ago by chapoton

Just for information, the doctest in heegner is also touched by #25635.

comment:20 Changed 2 years ago by gh-timokau

  • Cc gh-timokau added

comment:21 Changed 2 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from needs_review to needs_work
  • Summary changed from Upgrade to pari-2.10.0.alpha to Upgrade to pari-2.10.1.beta

comment:22 Changed 2 years ago by git

  • Commit changed from a62587a08668adf537fb3fcbb418b39dd8508b1f to c648214b288f031c9771bb8c021f62df814e68cd

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

864785aUpgrade to pari-2.10.1.beta
c648214Fix doctests for PARI/GP upgrade

comment:23 Changed 2 years ago by jdemeyer

Unfortunately, this breaks cypari2:

[cypari-1.1.4]     cypari2/gen.c: In function '__pyx_pf_7cypari2_3gen_3Gen_232_nf_nfzk':
[cypari-1.1.4]     cypari2/gen.c:157941:3: error: too many arguments to function 'nf_nfzk'
[cypari-1.1.4]        nf_nfzk(__pyx_v_self->__pyx_base.g, __pyx_v_t0->__pyx_base.g, (&__pyx_v_zknf), (&__pyx_v_czknf));
[cypari-1.1.4]        ^~~~~~~
[cypari-1.1.4]     In file included from ../../usr/local/src/sage-config/local/include/pari/pari.h:45:0,
[cypari-1.1.4]                      from cypari2/gen.c:599:
[cypari-1.1.4]     ../../usr/local/src/sage-config/local/include/pari/paridecl.h:2329:5: note: declared here
[cypari-1.1.4]      GEN     nf_nfzk(GEN nf, GEN rnfeq);
[cypari-1.1.4]          ^~~~~~~
[cypari-1.1.4]     cypari2/gen.c: In function '__pyx_pf_7cypari2_3gen_3Gen_234_nfeltup':
[cypari-1.1.4]     cypari2/gen.c:158154:60: error: too many arguments to function 'nfeltup'
[cypari-1.1.4]        __pyx_t_1 = ((PyObject *)__pyx_f_7cypari2_5stack_new_gen(nfeltup(__pyx_v_self->__pyx_base.g, __pyx_v_t0->__pyx_base.g, __pyx_v_t1->__pyx_base.g, __pyx_v_t2->__pyx_base.g))); if (unlikely(!__pyx_t_1)) __PYX_ERR(2, 4064, __pyx_L1_error)
[cypari-1.1.4]                                                                 ^~~~~~~
[cypari-1.1.4]     In file included from ../../usr/local/src/sage-config/local/include/pari/pari.h:45:0,
[cypari-1.1.4]                      from cypari2/gen.c:599:
[cypari-1.1.4]     ../../usr/local/src/sage-config/local/include/pari/paridecl.h:2332:5: note: declared here
[cypari-1.1.4]      GEN     nfeltup(GEN nf, GEN x, GEN zknf);
[cypari-1.1.4]          ^~~~~~~

comment:24 Changed 2 years ago by jdemeyer

  • Dependencies set to #25813

comment:25 Changed 2 years ago by git

  • Commit changed from c648214b288f031c9771bb8c021f62df814e68cd to b85a7fdad6ec8e5d23119ed310852f31bf8b770a

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

da380b3Upgrade cypari2
08ee923Upgrade to pari-2.10.1.beta
b85a7fdFix doctests for PARI/GP upgrade

comment:26 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:27 Changed 2 years ago by git

  • Commit changed from b85a7fdad6ec8e5d23119ed310852f31bf8b770a to 58bde2ea1b5b031f61fbb7de771c578f4635a66b

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

62fe6ebUpgrade cypari2
18ffa7eUpgrade to pari-2.10.1.beta
58bde2eFix doctests for PARI/GP upgrade

comment:28 Changed 2 years ago by jdemeyer

  • Dependencies changed from #25813 to #25813, #25635

comment:29 Changed 2 years ago by git

  • Commit changed from 58bde2ea1b5b031f61fbb7de771c578f4635a66b to 8bdbac586df58b9b55079465dba11323c6d9a7f8

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

61b0930possibly better make_monic in heegner.py
9cbb66bImprove make_monic
6995e7cMerge commit '9cbb66bf5822c77fb8356af27ccc2d03401d0b25' into t/25567/upgrade_to_pari_2_10_0_alpha
adf99c0Upgrade to pari-2.10.1.beta
8bdbac5Fix doctests for PARI/GP upgrade

comment:30 Changed 2 years ago by git

  • Commit changed from 8bdbac586df58b9b55079465dba11323c6d9a7f8 to 5e40504d977c0e9ecf4ab494715667d1e61a7408

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

5e40504Fix doctests for PARI/GP upgrade

comment:31 Changed 2 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:32 Changed 2 years ago by gh-timokau

pari-2.11.0 (STABLE) released !

Should we instead update to that or first update to beta and then follow up?

comment:33 Changed 2 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from needs_review to needs_work
  • Summary changed from Upgrade to pari-2.10.1.beta to Upgrade to pari-2.11

comment:34 Changed 2 years ago by gh-timokau

I'm experiencing a giac test failure when I update the pari nix package. I'm not sure yet if its actually caused by the pari update, the giac test suite seems somewhat non-deterministic. I'll investigate and report upstream if it is relevant.

comment:35 follow-up: Changed 2 years ago by gh-timokau

Seems like I get the failure

13c13
< 1,
---
> matrix[[2,7,1],[3,2,1],[389,2,1],[733,2,1],[156904374622257604823879982847602392900751802349981470895277241,2,matrix>
FAIL: chk_fhan11

when updating pari. The latest version of giac (1.4.9-69) unfortunately doesn't fix this but instead introduces one more failure:

35c35
< 2/3*sign(a)*asin(sqrt(x)*x/sqrt(a)/a),
---
> 2/3*asin(sqrt(x)*x/sqrt(a)/a)*sign(a),
FAIL: chk_integrate

I'll have to wait until my upstream forum account is approved until I can report.

comment:36 Changed 2 years ago by git

  • Commit changed from 5e40504d977c0e9ecf4ab494715667d1e61a7408 to 7af4748cab37d651eaa88be501db88f4a5ffc584

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

d867a8dUpgrade to pari-2.11.0
7af4748Fix doctests for PARI/GP upgrade

comment:37 Changed 2 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:38 in reply to: ↑ 35 Changed 2 years ago by jdemeyer

Replying to gh-timokau:

Seems like I get the failure

13c13
< 1,
---
> matrix[[2,7,1],[3,2,1],[389,2,1],[733,2,1],[156904374622257604823879982847602392900751802349981470895277241,2,matrix>
FAIL: chk_fhan11

when updating pari.

Did you report that upstream?

I analyzed the problem and the problem happens because the output of isprime(p, 1) changed from returning a primality certificate to a boolean answer. There is now a separate function primecert() for getting a certificate.

The test directly checks the PARI interface, but otherwise these primality certificates are not used by anything in giac. So while the test fails, nothing else in giac should fail because of this.

comment:39 in reply to: ↑ 39 ; follow-ups: Changed 2 years ago by gh-timokau

Replying to jdemeyer:

Replying to gh-timokau:

Seems like I get the failure

13c13
< 1,
---
> matrix[[2,7,1],[3,2,1],[389,2,1],[733,2,1],[156904374622257604823879982847602392900751802349981470895277241,2,matrix>
FAIL: chk_fhan11

when updating pari.

Did you report that upstream?

The only contact info I found was that forum and I'm still waiting for my account to be approved. I think I accidentally chose the wrong option at the age question (doesn't help that the forum is in french). I sent an email to the administrators, no reply yet.

If you want to try, maybe you have more luck. I wish they'd just use some issue tracker or plain old email.

I analyzed the problem and the problem happens because the output of isprime(p, 1) changed from returning a primality certificate to a boolean answer. There is now a separate function primecert() for getting a certificate.

The test directly checks the PARI interface, but otherwise these primality certificates are not used by anything in giac. So while the test fails, nothing else in giac should fail because of this.

Great, thanks for analyzing this :)

Should we then adapt / disable giac's spkg-check until this is fixed? Or is it okay for those spkg checks to fail?

comment:40 in reply to: ↑ 39 Changed 2 years ago by jdemeyer

  • Cc frederichan added

Replying to gh-timokau:

I wish they'd just use some issue tracker or plain old email.

More generally, their website is a mess... Even finding the source tarballs is hard. And that page has debian in its name even though these are just the sources: https://www-fourier.ujf-grenoble.fr/~parisse/debian/dists/stable/main/source/

I just recall now that @frederichan is a Giac developer and usually all bug reports from Sage go through him. ​Frederic: if you missed the discussion, the upgrade to PARI/GP breaks one test from Giac:

13c13
< 1,
---
> matrix[[2,7,1],[3,2,1],[389,2,1],[733,2,1],[156904374622257604823879982847602392900751802349981470895277241,2,matrix>
FAIL: chk_fhan11

See 38 for the analysis.

comment:41 follow-up: Changed 2 years ago by frederichan

I have reported this here: https://xcas.univ-grenoble-alpes.fr/forum/viewtopic.php?f=4&t=2102#p10326

As this example is just calling the pari function, patching the output with the new behaviour looks natural, something like this?

--- a/check/TP11-sol.cas.out1	2018-07-26 17:12:30.516949491 +0200
+++ b/check/TP11-sol.cas.out1	2018-07-26 17:13:04.397032363 +0200
@@ -10,7 +10,7 @@
 1073741824000000000000000000061203284109000000000000000000000000008409,
 2^3*3*389*733*156904374622257604823879982847602392900751802349981470895277241,
 "Done",
-matrix[[2,7,1],[3,2,1],[389,2,1],[733,2,1],[156904374622257604823879982847602392900751802349981470895277241,2,matrix[[2,13,1],[3,3,1],[5,2,1],[7,2,1],[56467,2,1],[6553084925887974620811527,2,matrix[[2,5,1],[19,2,1],[71,2,1],[126823,2,1]]]]]],
+1,
 0,
 [],
 1,

best Frederic

comment:42 Changed 2 years ago by gh-timokau

Thanks for reporting! If the test really just tests the pari interface without ever using that part of the interface, I'd just remove the test instead.

Not quite relevant to this ticket, but could you report the chk_integrate failure too? the test failure is here. That occurs when I update giac from 1.4.9-59 to 1.4.9-69 (without changing anything else).

comment:43 in reply to: ↑ 41 Changed 2 years ago by jdemeyer

Replying to frederichan:

I have reported this here: https://xcas.univ-grenoble-alpes.fr/forum/viewtopic.php?f=4&t=2102#p10326

As this example is just calling the pari function, patching the output with the new behaviour looks natural, something like this?

Depends whether you want to want the tests to pass on older PARI versions too. Personally, I would just remove the test completely.

comment:44 follow-up: Changed 2 years ago by gh-timokau

@frederichan also not really relevant for this ticket, but lacking another communication channel: Apparently the nix maintainer for giac also already requested a forum account last yer and never got it: https://github.com/NixOS/nixpkgs/pull/44129#issuecomment-408203217

Regarding this ticket: I don't know how the review process works, but LGTM. I haven't run the doctests yet though.

comment:45 in reply to: ↑ 44 Changed 2 years ago by jdemeyer

Replying to gh-timokau:

I don't know how the review process works

Just a general reply:

  • look at the diff and see whether it makes sense.
  • Sage cares a lot about tests, so check whether the code is well tested.
  • check the patchbot (the colored blob at the top of the page).
  • maybe try a few examples, test corner cases in the code.
  • if all is well: set the ticket to positive_review and add your real name as Reviewer on the Trac page. At that point, the release manager will merge the ticket in some later beta/rc release (assuming that all dependencies are merged).

In this case, this is a package upgrade, which is a bit different. The patchbot doesn't test it, so somebody should manually run the full Sage testsuite. If you want to take my word for it, I did that and tests passed.

comment:46 Changed 2 years ago by frederichan

I have also reported chk_integrate here: https://xcas.univ-grenoble-alpes.fr/forum/viewtopic.php?f=4&t=2104

Indeed my giac/xcas forum account is old, to obtain one I think the safest is a direct mail to Bernard Parisse: https://www-fourier.ujf-grenoble.fr/?q=fr/content/annuaire (he may be in vacation at this time). He also have a trac account: parisse

(cf https://trac.sagemath.org/ticket/25822#comment:12)

Regarding the testsuite, although many files have my name I didn't wrote them for a testsuite so they are often too sensitive. But I think (this was often the case in the past) that bernard prefers to keep many examples and look if the changes are bugs or not. In this particular example, there are not so many giac functions that can't be run without pari (ex: ifactor still works in giac without pari), so I guess that bernard will keep this test.

For a sage package where you always have pari you may decide to suppress it.

NB: if you plan to upgrade giac package, 1.4.6.69 is not enough to fix #25822 but the next stable should be.

comment:47 follow-up: Changed 2 years ago by gh-timokau

Replying to jdemeyer:

Replying to gh-timokau:

I don't know how the review process works

Just a general reply:

  • look at the diff and see whether it makes sense.

Did that. While I won't pretend all the math, the changes look very reasonable to me. No obviously wrong results and I think we can trust pari with the details.

Two things that stuck out to me though:

sage: a.multiplicative_order()
-        4547473508864641189575195312
+        189478062869360049565633138
             sage: O3.absolute_discriminant()
-            8643600
+            400160824478095086350656915693814563600

I would've thought that both the multiplicative order and the absolute discriminant are supposed to be unique. But again, my knowledge is limited.

  • Sage cares a lot about tests, so check whether the code is well tested.

check

  • check the patchbot (the colored blob at the top of the page).

Oh, I've always wondered what exactly the patchbot is everybody is talking about.

  • maybe try a few examples, test corner cases in the code.
  • if all is well: set the ticket to positive_review and add your real name as Reviewer on the Trac page. At that point, the release manager will merge the ticket in some later beta/rc release (assuming that all dependencies are merged).

But can anybody just do that?

In this case, this is a package upgrade, which is a bit different. The patchbot doesn't test it, so somebody should manually run the full Sage testsuite. If you want to take my word for it, I did that and tests passed.

Why doesn't the patchbot test package upgrades? I think if we can just take anybody's word for it, its probably yours.

Replying to frederichan:

I have also reported chk_integrate here: https://xcas.univ-grenoble-alpes.fr/forum/viewtopic.php?f=4&t=2104

Thanks :)

Regarding the testsuite, although many files have my name I didn't wrote them for a testsuite so they are often too sensitive. But I think (this was often the case in the past) that bernard prefers to keep many examples and look if the changes are bugs or not. In this particular example, there are not so many giac functions that can't be run without pari (ex: ifactor still works in giac without pari), so I guess that bernard will keep this test.

A very brittle test suite is at risk of just being ignored after a few false-positives. I think a few high-value tests are way more valuable than testing the literal output of every function. Sage has a bit of the same problem in my opinion.

For a sage package where you always have pari you may decide to suppress it.

For now I just did echo > check/chk_fhan11. Its not that easy to disable an individual test.

comment:48 in reply to: ↑ 47 Changed 2 years ago by jdemeyer

Replying to gh-timokau:

Two things that stuck out to me though:

sage: a.multiplicative_order()
-        4547473508864641189575195312
+        189478062869360049565633138
             sage: O3.absolute_discriminant()
-            8643600
+            400160824478095086350656915693814563600

I would've thought that both the multiplicative order and the absolute discriminant are supposed to be unique.

The multiplicative order is certainly unique, but the element a that we're taking the order of is different.

In the second case, I changed the test input because PARI got more clever in computing orders. We need the large primes 1000003 and 1000099 to make it more difficult for PARI to accidentally find the maximal order.

  • check the patchbot (the colored blob at the top of the page).

Oh, I've always wondered what exactly the patchbot is everybody is talking about.

It's what Sage uses for continuous integration. It's far from perfect and there is a proposal to replace it: #24854

But can anybody just do that?

Yes, anybody can do that. This process seems to be working fine.

Why doesn't the patchbot test package upgrades?

Good question actually, I don't know the answer.

comment:49 Changed 2 years ago by gh-timokau

  • Status changed from needs_review to positive_review

Oh, I didn't notice that you also changed the test input.

comment:50 Changed 2 years ago by tscrim

Reviewer name.

comment:51 Changed 2 years ago by gh-timokau

  • Reviewers set to Timo Kaufmann

comment:52 follow-up: Changed 2 years ago by vbraun

FWIW the testsuite fails on kucalc (linux 64-bit):

* Testing galpol           gp-sta..BUG [2116]   gp-dyn..BUG [2220]   

comment:53 follow-ups: Changed 2 years ago by vbraun

Pretty sure this is due to pari on 32-bit linux:

File "src/sage/schemes/elliptic_curves/period_lattice.py", line 1463, in sage.schemes.elliptic_curves.period_lattice.PeriodLattice_ell.elliptic_logarithm
Failed example:
    L.elliptic_exponential(_)
Expected:
    (3.00000000000000000000000000... : 5.00000000000000000000000000... : 1.000000000000000000000000000)
Got:
    (3.000000000000000000000000000 : 4.999999999999999999999999999 : 1.000000000000000000000000000)

I also get a lot of buildbots with giac testsuite failures, this might also be due to the pari udpate. I'll test some more... (edit: i see its already noted here)

Last edited 2 years ago by vbraun (previous) (diff)

comment:54 follow-up: Changed 2 years ago by vbraun

PS: the patchbot doesn't test package updates since it doesn't have a programmatic way of downloading tarballs, also patchbot does incremental rebuilds which are likely to get hosed if something is wrong with the ticket.

comment:55 Changed 2 years ago by jdemeyer

So should this be set to needs_work then?

comment:56 in reply to: ↑ 53 Changed 2 years ago by cremona

Replying to vbraun:

Pretty sure this is due to pari on 32-bit linux:

File "src/sage/schemes/elliptic_curves/period_lattice.py", line 1463, in sage.schemes.elliptic_curves.period_lattice.PeriodLattice_ell.elliptic_logarithm
Failed example:
    L.elliptic_exponential(_)
Expected:
    (3.00000000000000000000000000... : 5.00000000000000000000000000... : 1.000000000000000000000000000)
Got:
    (3.000000000000000000000000000 : 4.999999999999999999999999999 : 1.000000000000000000000000000)

That makes sense. In line 1773 the pari function ellwp() is called (in the method elliptic_exponential()) and that is what does the work for this.

I also get a lot of buildbots with giac testsuite failures, this might also be due to the pari udpate. I'll test some more... (edit: i see its already noted here)

comment:57 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

Also I'd appreciate it if the giac testsuite could be worked around, otherwise I'll have to disabled it on the buildbot and, realistically, I'll never turn it back on.

comment:58 Changed 2 years ago by git

  • Commit changed from 7af4748cab37d651eaa88be501db88f4a5ffc584 to e7b4a3708094a396bd2d4924b212786365464b76

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

92c639aUpgrade to pari-2.11.0
e7b4a37Fix doctests for PARI/GP upgrade

comment:59 Changed 2 years ago by jdemeyer

  • Dependencies changed from #25813, #25635 to #26010

comment:60 in reply to: ↑ 54 Changed 2 years ago by jdemeyer

Replying to vbraun:

patchbot does incremental rebuilds which are likely to get hosed if something is wrong with the ticket.

That's true for any ticket. I don't believe that package upgrades are the tickets most likely to cause breakage of the patchbot.

comment:61 in reply to: ↑ 52 Changed 2 years ago by jdemeyer

Replying to vbraun:

FWIW the testsuite fails on kucalc (linux 64-bit):

* Testing galpol           gp-sta..BUG [2116]   gp-dyn..BUG [2220]   

Fixed by #26010.

comment:62 in reply to: ↑ 53 Changed 2 years ago by jdemeyer

Replying to vbraun:

Pretty sure this is due to pari on 32-bit linux:

Fixed by adding some doctest tolerance.

comment:63 Changed 2 years ago by git

  • Commit changed from e7b4a3708094a396bd2d4924b212786365464b76 to 9e1419f8052d3e9921a178b20e2d31ff8252805e

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

197cabbSplit database_pari
1c1f18dUpgrade to pari-2.11.0
9e1419fFix doctests for PARI/GP upgrade

comment:64 Changed 2 years ago by git

  • Commit changed from 9e1419f8052d3e9921a178b20e2d31ff8252805e to 6969b7430ab52b58f65334fa12e78433ce010421

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

6969b74Add giac patch for PARI 2.11

comment:65 Changed 2 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:66 Changed 2 years ago by jdemeyer

Ping? It would be nice to review this again. I recall that it had positive_review before. So this would need reviewing the dependency #26010 and the few added commits.

comment:67 Changed 2 years ago by git

  • Commit changed from 6969b7430ab52b58f65334fa12e78433ce010421 to 739f7a6acf98e7041265208836a85ab69573b36c

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

1b3371cPARI databases have no dependencies
21ba754Add giac patch for PARI 2.11
36574c3Upgrade to pari-2.11.0
739f7a6Fix doctests for PARI/GP upgrade

comment:68 Changed 2 years ago by jdemeyer

Rebased to latest version of #26010.

comment:69 Changed 2 years ago by vbraun

  • Status changed from needs_review to positive_review

comment:70 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/upgrade_to_pari_2_10_0_alpha to 739f7a6acf98e7041265208836a85ab69573b36c
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:71 follow-ups: Changed 2 years ago by cremona

  • Commit 739f7a6acf98e7041265208836a85ab69573b36c deleted

Since the release of 2.11 there have been several bug-fixes which are important for my current work. Some of the new mf* functions crash on 2.11.0 but run fine with the current development commit. What I am currently doing is building this development gp executable outside Sage and manually changing the link SAGE_ROOT/local/bin/gp to point to that. (My code is only using gp.eval(), not Pari library functions.

  1. Is there a better way?
  2. Is the version going to be updated regularly to the current development version of pari?

comment:72 in reply to: ↑ 71 Changed 2 years ago by fbissey

Replying to cremona:

Since the release of 2.11 there have been several bug-fixes which are important for my current work. Some of the new mf* functions crash on 2.11.0 but run fine with the current development commit. What I am currently doing is building this development gp executable outside Sage and manually changing the link SAGE_ROOT/local/bin/gp to point to that. (My code is only using gp.eval(), not Pari library functions.

  1. Is there a better way?

As far as I can say that's quick and slightly dirty but probably not too much. I think the best way would be for your code to look for a variable, say GP_EXEC, and use its value as the gp executable if it exists. You can then put that in your environment and only your code is ever affected and you don't need to mess up with the link.

comment:73 Changed 2 years ago by cremona

I thought of something like that but the Gp() constructor for a new gp interpreter does not allow the user to specify the interpreter's path explicitly. It calls Expect.__init__ with command="gp --fast --emacs --quiet --stacksize %s"%stacksize so that relies on picking up gp from Sage's path.

Am I missing something? I am using a personal Sage build rather than messing with the system-wide one.

comment:74 in reply to: ↑ 71 Changed 2 years ago by gh-timokau

Replying to cremona:

Since the release of 2.11 there have been several bug-fixes which are important for my current work. Some of the new mf* functions crash on 2.11.0 but run fine with the current development commit. What I am currently doing is building this development gp executable outside Sage and manually changing the link SAGE_ROOT/local/bin/gp to point to that. (My code is only using gp.eval(), not Pari library functions.

  1. Is there a better way?

sage sets its PATH in sage-env. After setting that it sources ~/.sage/sagerc if available. So if you create that file with the following contents:

export PATH="<insert path to local build of gp here>/bin:$PATH"

sage should pick up your gp since it is first in PATH.

  1. Is the version going to be updated regularly to the current development version of pari?

I don't think that is a good idea. Development versions are not for "production" use. Instead you could ask upstream for a release or suggest to them to release bugfixes in point releases.

comment:75 Changed 2 years ago by gh-timokau

In the long run you may also be interested in #24919.

comment:76 follow-up: Changed 2 years ago by cremona

Thanks for the suggestion in 1 which is certainly cleaner than whan I was doing. As for 2, it is a matter of terminology what is production and what is testing. The work I am doing is effectively a massive test of the new modular forms functionality in Pari/GP, and I am finding bugs which upstream Pari is fixing. But I am doing the testing using calls from Sage since using gp itself is much harder, especially when I get to compare the results with the output of a different package (hint: one of the 3 Ms).

Of course that is not to say that Sage should be repeatedly changing to the latest pari commit; however pari releases come around rather too infrequently.

comment:77 in reply to: ↑ 76 ; follow-up: Changed 2 years ago by gh-timokau

Replying to cremona:

Thanks for the suggestion in 1 which is certainly cleaner than whan I was doing. As for 2, it is a matter of terminology what is production and what is testing. The work I am doing is effectively a massive test of the new modular forms functionality in Pari/GP, and I am finding bugs which upstream Pari is fixing. But I am doing the testing using calls from Sage since using gp itself is much harder, especially when I get to compare the results with the output of a different package (hint: one of the 3 Ms).

In that case the current solution or one through #24919 is probably best.

Of course that is not to say that Sage should be repeatedly changing to the latest pari commit; however pari releases come around rather too infrequently.

Yes that is true. But if there is something upstream we need, we should at least ask them for a release before deciding to just fetch some unstable commit.

comment:78 in reply to: ↑ 77 Changed 2 years ago by jdemeyer

Replying to gh-timokau:

Yes that is true. But if there is something upstream we need, we should at least ask them for a release before deciding to just fetch some unstable commit.

We can ask, but it probably won't happen just because we ask.

comment:79 Changed 2 years ago by gh-timokau

We should still ask. Even if it doesn't happen, they may have good reasons for that and explain them.

comment:80 Changed 2 years ago by cremona

Bother @jdemeyer and I are in regular contact with the Pari developers anyway.

comment:81 follow-up: Changed 2 years ago by gh-timokau

Having the reasons public would be better. Some explanation why the state of that particular comment is good enough for sage but not good enough for pari's upstream.

All that is hypothetical in the case that pari adds important features that we need in sage and they refuse to make a release including them in reasonable time. As far as I understand, that is not the case yet.

comment:82 in reply to: ↑ 81 Changed 2 years ago by jdemeyer

Replying to gh-timokau:

All that is hypothetical in the case that pari adds important features that we need in sage and they refuse to make a release including them in reasonable time. As far as I understand, that is not the case yet.

It's not the case yet, but judging from past experiences, this is likely to happen.

Note: See TracTickets for help on using tickets.