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

Make Sage compatible with PARI stable and PARI master #23796

Closed
jdemeyer opened this issue Sep 7, 2017 · 20 comments
Closed

Make Sage compatible with PARI stable and PARI master #23796

jdemeyer opened this issue Sep 7, 2017 · 20 comments

Comments

@jdemeyer
Copy link

jdemeyer commented Sep 7, 2017

This makes a few changes to Sage to allow it to be compatible with both PARI stable 2.9.3, as well as PARI git master (see #23544).

With "compatible", I mean that Sage builds and that all doctests are mathematically correct. So, doctests might fail but only because one correct answer is replaced by a different correct answer.

The main change is one call to ellwp(), where the current PARI version returns a value which is a factor 2 too small. Interestingly, Sage compensates for this by using a wrong formula later, to get correct results anyway.

CC: @infinity0 @kiwifb @JohnCremona

Component: packages: standard

Author: Jeroen Demeyer

Branch/Commit: c214994

Reviewer: François Bissey

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

@jdemeyer jdemeyer added this to the sage-8.1 milestone Sep 7, 2017
@jdemeyer
Copy link
Author

jdemeyer commented Sep 7, 2017

Branch: u/jdemeyer/ticket/23796

@kiwifb
Copy link
Member

kiwifb commented Sep 7, 2017

Commit: 108ff2e

@kiwifb
Copy link
Member

kiwifb commented Sep 7, 2017

New commits:

a668387Fix elliptic_exponential()
3d582ecDon't use undocumented nf[7] in Simon's scripts
108ff2eUse "abs tol" for clarity

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

jdemeyer commented Sep 7, 2017

New commits:

a668387Fix elliptic_exponential()
3d582ecDon't use undocumented nf[7] in Simon's scripts
108ff2eUse "abs tol" for clarity

@jdemeyer

This comment has been minimized.

@kiwifb
Copy link
Member

kiwifb commented Sep 13, 2017

comment:6

Looks good to me. Passes basic tests.

@kiwifb
Copy link
Member

kiwifb commented Sep 13, 2017

Reviewer: Francois Bissey

@jdemeyer
Copy link
Author

comment:7

Thanks! I hope this helps.

@jdemeyer
Copy link
Author

Changed reviewer from Francois Bissey to François Bissey

@vbraun
Copy link
Member

vbraun commented Sep 17, 2017

comment:8

On 32-bit:

File "src/sage/schemes/elliptic_curves/period_lattice.py", line 136, in sage.schemes.elliptic_curves.period_lattice._ellwp_flag1
Failed example:
    _ellwp_flag1(E, E((0,1)).elliptic_logarithm())
Expected:
    (-7.71860259319095 E-30, 2.00000000000000)
Got:
    (-7.71860260 E-30, 2.00000000000000)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2017

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

d80d58332-bit doctest fix

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2017

Changed commit from 108ff2e to d80d583

@jdemeyer
Copy link
Author

comment:10

Obvious fix.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2017

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

c21499432-bit doctest fix

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 18, 2017

Changed commit from d80d583 to c214994

@jdemeyer jdemeyer removed this from the sage-8.1 milestone Sep 19, 2017
@jdemeyer
Copy link
Author

comment:14

Pending because of upstream discussion at sagemath/cypari2#38

@jdemeyer
Copy link
Author

comment:15

OK, the ellwp() bug is now also fixed upstream in cypari2. The simplest way forward is probably to merge this ticket as-is and then revert the ellwp() fix in Sage when we upgrade cypari2.

@jdemeyer jdemeyer added this to the sage-8.1 milestone Sep 20, 2017
@jdemeyer jdemeyer removed the pending label Sep 20, 2017
@kiwifb
Copy link
Member

kiwifb commented Sep 20, 2017

comment:16

Because you had left the ticket on positive review Volker already included it in his current merge, we can expect it to be closed fixed in his next round of closure - unless some new problem crop up.

@vbraun
Copy link
Member

vbraun commented Sep 20, 2017

Changed branch from u/jdemeyer/ticket/23796 to c214994

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

3 participants