Skip to content
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 #25567

Closed
jdemeyer opened this issue Jun 12, 2018 · 95 comments
Closed

Upgrade to pari-2.11 #25567

jdemeyer opened this issue Jun 12, 2018 · 95 comments

Comments

@jdemeyer
Copy link

Tarball: http://pari.math.u-bordeaux.fr/pub/pari/unix/pari-2.11.0.tar.gz

Depends on #26010

Upstream: Fixed upstream, but not in a stable release.

CC: @kiwifb @JohnCremona @timokau @frederichan-IMJPRG

Component: packages: standard

Author: Jeroen Demeyer

Branch: 739f7a6

Reviewer: Timo Kaufmann

Issue created by migration from https://trac.sagemath.org/ticket/25567

@jdemeyer
Copy link
Author

@JohnCremona
Copy link
Member

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

@JohnCremona
Copy link
Member

Commit: 9ed413d

@JohnCremona
Copy link
Member

comment:4

Attachment: pols.sage.gz

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 13, 2018

Changed commit from 9ed413d to 68662b1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 13, 2018

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

68662b1Fix doctests for PARI/GP upgrade

@jdemeyer
Copy link
Author

comment:6

Replying to @JohnCremona:

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.

@JohnCremona
Copy link
Member

comment:7

Replying to @jdemeyer:

Replying to @JohnCremona:

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.

@jdemeyer
Copy link
Author

Upstream: Reported upstream. No feedback yet.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

comment:8

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.

@JohnCremona
Copy link
Member

comment:9

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

@jdemeyer
Copy link
Author

comment:10

Replying to @JohnCremona:

I could not find pari bug 2055

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

@jdemeyer

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 13, 2018

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 13, 2018

Changed commit from 68662b1 to e8cd3ab

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 13, 2018

Changed commit from e8cd3ab to cceabac

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 13, 2018

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

@jdemeyer
Copy link
Author

comment:16

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2018

Changed commit from cceabac to a62587a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 14, 2018

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

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

Changed upstream from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.

@jdemeyer
Copy link
Author

jdemeyer commented Aug 7, 2018

comment:61

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.

@jdemeyer
Copy link
Author

jdemeyer commented Aug 7, 2018

comment:62

Replying to @vbraun:

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

Fixed by adding some doctest tolerance.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 7, 2018

Changed commit from e7b4a37 to 9e1419f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 7, 2018

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 7, 2018

Changed commit from 9e1419f to 6969b74

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 7, 2018

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

6969b74Add giac patch for PARI 2.11

@jdemeyer
Copy link
Author

comment:66

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 22, 2018

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 22, 2018

Changed commit from 6969b74 to 739f7a6

@jdemeyer
Copy link
Author

comment:68

Rebased to latest version of #26010.

@vbraun
Copy link
Member

vbraun commented Aug 26, 2018

Changed branch from u/jdemeyer/upgrade_to_pari_2_10_0_alpha to 739f7a6

@JohnCremona
Copy link
Member

Changed commit from 739f7a6 to none

@JohnCremona
Copy link
Member

comment:71

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?

@kiwifb
Copy link
Member

kiwifb commented Sep 7, 2018

comment:72

Replying to @JohnCremona:

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.

@JohnCremona
Copy link
Member

comment:73

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.

@timokau
Copy link
Contributor

timokau commented Sep 7, 2018

comment:74

Replying to @JohnCremona:

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.

@timokau
Copy link
Contributor

timokau commented Sep 7, 2018

comment:75

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

@JohnCremona
Copy link
Member

comment:76

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.

@timokau
Copy link
Contributor

timokau commented Sep 7, 2018

comment:77

Replying to @JohnCremona:

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.

@jdemeyer
Copy link
Author

jdemeyer commented Sep 7, 2018

comment:78

Replying to @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.

@timokau
Copy link
Contributor

timokau commented Sep 7, 2018

comment:79

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

@JohnCremona
Copy link
Member

comment:80

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

@timokau
Copy link
Contributor

timokau commented Sep 7, 2018

comment:81

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.

@jdemeyer
Copy link
Author

jdemeyer commented Sep 7, 2018

comment:82

Replying to @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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants