-
-
Notifications
You must be signed in to change notification settings - Fork 514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade to pari 2.11.4 #29313
Comments
comment:1
Already updated on Arch. Some of the failures are actual incorrect results which are due to a pari bug fixed in [1]. The remaining ones are due to changes in chosen ideal generators, and in computation precision. Those just need the doctests to be updated. Will post a branch when I have some free time unless someone beats me to it. |
comment:2
I just now noticed #28424 (missed it initially because I searched for pari 2.11.3 in particular). That ticket proposes an update to 2.12 though, which is still in beta and probably harder to do. So I think we can keep this open. |
comment:3
Replying to @antonio-rojas:
Thats great news, amazing how fast you always come up with those fixes :) For anyone else wondering, here is Antonio's patch. |
comment:4
Since Sage can now use system pari, perhaps 2.11.3 should be blacklisted until this is merged. Ideally, distros shipping 2.11.3 should backport the patch as it is quite a serious bug. |
comment:5
Oh, I misread your original comment and I thought that you meant the tests were wrong before and 2.11.3 fixes the bug. I'll backport the pari fix for nix. |
comment:6
Pari 2.11.3 has been released just few days ago, and that bug was fixed last year. Could it be that upstream forgot to include the fix? What's an example of incorrect maths output without that patch? |
comment:7
Replying to @dimpase:
Yes, they forgot to backport the fix to stable. See [1]
You have some examples in [1] and [2] [1] https://pari.math.u-bordeaux.fr/cgi-bin/bugreport.cgi?bug=2208 [2] https://pari.math.u-bordeaux.fr/cgi-bin/bugreport.cgi?bug=2164 This causes a few failures in Sage. Among other things, simon's scripts are totally broken. |
comment:8
When patching pari with [1] and sage with [2] I still get at least one doctest failure (I only tested the number_fields.py file for now):
Am I missing something? |
comment:9
Given that this seem to be a serious bug, can we expect upstream to release 2.11.4 shortly to fix it? |
comment:10
Replying to @timokau:
That's one of the failures that should be fixed with pari's patch, are you sure you're running the patched version? |
comment:11
No, I wasn't. I had a typo ("patch" instead of "patches") that resulted in the patch silently being ignored. Sorry for the noise. As an aside, the upstream patch doesn't quite apply cleanly but the arch patch [1] is rebased. Thanks! |
Branch: u/arojas/upgrade_to_pari_2_11_3 |
Commit: |
comment:13
Needs testing for the check_functional_equation() output on 32 bits, i've only checked 64 bits. Also, some elliptic curves expert should check if the changes in elliptic_curves/ell_number_field.py are mathematically correct. All other ones seem OK to me. New commits:
|
Author: Antonio Rojas |
comment:14
The way I understand the configuration, once this ticket is through, it will reject a system pari unless it is at least version 2.11.3. Is this correct? |
comment:15
Replying to @kliem:
No, the minimum pari system version is defined in spkg-configure and is independent of the version bundled in sage |
comment:16
Ok, then I get why you want sage's test to be work with pari 2.11.3 and previous versions. Do you want this to be done in this ticket or in a seperate ticket? |
comment:17
Replying to @kliem:
I don't really mind as long as I can get rid of my downstream patch. Feel free to take over this branch and modify the test results so they pass with both versions. |
Dependencies: #29472 |
Branch pushed to git repo; I updated commit sha1. New commits:
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
comment:25
please also apply --- a/build/pkgs/pari/checksums.ini
+++ b/build/pkgs/pari/checksums.ini
@@ -2,3 +2,4 @@ tarball=pari-VERSION.tar.gz
sha1=2b9ff51feb388664b834dc346a44867546c78618
md5=fb2968d7805424518fe44a59a2024afd
cksum=1247903778
+upstream_url=https://pari.math.u-bordeaux.fr/pub/pari/unix/pari-2.11.4.tar.gz |
Branch pushed to git repo; I updated commit sha1. New commits:
|
This comment has been minimized.
This comment has been minimized.
comment:28
some tests on macOS and Cygwin would be great. Perhaps adapting GH Actions to restrict platforms... |
comment:30
for some reason I don't get macOS runs on GH Actions any more. Is it by design, or GH pulled them? |
comment:31
Replying to @dimpase:
No, works just fine for me -- for example here: https://github.com/mkoeppe/sage/actions/runs/141135018 |
comment:32
You probably need to cancel some of your old queued workflows such as this one: https://github.com/dimpase/sage/runs/784000040?check_suite_focus=true |
comment:33
What's missing on this ticket? |
comment:34
basically, someone should bite the bullet and make pari-related doctests less rigid (cf. e.g. the 1st example in the ticket description - it assumes an output order which is not really fixed),so that different versions of pari would still pass. |
comment:35
Replying to @dimpase:
That's done in #29472 already |
comment:36
ah, ok, then in principle it should be good to go. |
comment:37
Been using it for a little while in sage-on-gentoo and I don't need any patches for doctests anymore for some time now. So, I am putting this to positive. |
Changed reviewer from Dima Pasechnik to Dima Pasechnik, François Bissey |
Changed branch from u/arojas/upgrade_to_pari_2_11_3 to |
Unfortunately the update is not trivial. Here are some doctest failures:
I will not do the update myself in this ticket, I only opened it to notify people of the issues.
Tarfile: see checksums_ini [upstream_url]
Depends on #29472
CC: @antonio-rojas @kiwifb @saraedum @tobihan @fchapoton @embray @jdemeyer @dimpase
Component: packages: standard
Author: Antonio Rojas
Branch/Commit:
549e094
Reviewer: Dima Pasechnik, François Bissey
Issue created by migration from https://trac.sagemath.org/ticket/29313
The text was updated successfully, but these errors were encountered: