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

PARI discriminant speed depends on stack size #15654

Closed
jdemeyer opened this issue Jan 9, 2014 · 20 comments
Closed

PARI discriminant speed depends on stack size #15654

jdemeyer opened this issue Jan 9, 2014 · 20 comments

Comments

@jdemeyer
Copy link

jdemeyer commented Jan 9, 2014

This is weird and bad:

sage: x = polygen(ZpFM(3,10))
sage: p = ((x-1)^50 + x)._pari_init_()
sage: %time pari(p).poldisc()
CPU times: user 52.73 s, sys: 0.00 s, total: 52.73 s
Wall time: 52.82 s
2*3 + 3^4 + 2*3^6 + 3^7 + 2*3^8 + 2*3^9 + O(3^10)
sage: pari.allocatemem(2<<20)
PARI stack size set to 2097152 bytes
sage: %time pari(p).poldisc()
CPU times: user 0.08 s, sys: 0.00 s, total: 0.08 s
Wall time: 0.08 s
2*3 + 3^4 + 2*3^6 + 3^7 + 2*3^8 + 2*3^9 + O(3^10)

Upstream: http://pari.math.u-bordeaux.fr/cgi-bin/bugreport.cgi?bug=1507

Depends on #15653

Upstream: Reported upstream. Developers deny it's a bug.

CC: @pjbruin

Component: performance

Author: Jeroen Demeyer

Branch/Commit: u/jdemeyer/ticket/15654 @ a955e45

Reviewer: David Roe

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

@jdemeyer jdemeyer added this to the sage-6.1 milestone Jan 9, 2014
@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer changed the title Extremely slow PARI discriminants PARI discriminant speed depends on stack size Jan 9, 2014
@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

jdemeyer commented Jan 9, 2014

Upstream: Reported upstream. No feedback yet.

@jdemeyer
Copy link
Author

jdemeyer commented Jan 9, 2014

Dependencies: #15653

@jdemeyer
Copy link
Author

jdemeyer commented Jan 9, 2014

Author: Jeroen Demeyer

@jdemeyer
Copy link
Author

jdemeyer commented Jan 9, 2014

Changed upstream from Reported upstream. No feedback yet. to Reported upstream. Developers deny it's a bug.

@jdemeyer
Copy link
Author

jdemeyer commented Jan 9, 2014

Branch: u/jdemeyer/ticket/15654

@jdemeyer
Copy link
Author

jdemeyer commented Jan 9, 2014

Commit: a955e45

@jdemeyer
Copy link
Author

jdemeyer commented Jan 9, 2014

New commits:

7537427Fix conversion of zero p-adics to PARI
a955e45Speed up PARI determinants when stacksize is small

@jdemeyer
Copy link
Author

jdemeyer commented Jan 9, 2014

comment:8

This solution works well for Sage, maybe not for PARI/GP upstream.

@roed314
Copy link
Contributor

roed314 commented Jan 10, 2014

comment:9

I'm fine with this change. I'm not yet familiar with reviewing SPKG changes using the new directory layout. Why is SPKG.txt deleted in this commit?

More generally, are there other places in Sage where we should be more aggressive about increasing the Pari stack size? If someone is using Pari nontrivially, our current stack size seems too small. Should we increase the stack whenever a user does certain operations signaling that they're going to be using Pari extensively (e.g. create a number field of degree larger than 4, take the discriminant of a polynomial of large degree...)?

@jdemeyer
Copy link
Author

comment:10

Replying to @roed314:

I'm fine with this change. I'm not yet familiar with reviewing SPKG changes using the new directory layout. Why is SPKG.txt deleted in this commit?

Not all of SPKG.txt, just the changelog.

@jdemeyer
Copy link
Author

comment:11

Replying to @roed314:

More generally, are there other places in Sage where we should be more aggressive about increasing the Pari stack size? If someone is using Pari nontrivially, our current stack size seems too small. Should we increase the stack whenever a user does certain operations signaling that they're going to be using Pari extensively (e.g. create a number field of degree larger than 4, take the discriminant of a polynomial of large degree...)?

We could detect the problem by adding some code to gerepile...() to count the number of garbage collections. We could for example give a warning if more than N happen per second (for a suitable value of N).

@jdemeyer
Copy link
Author

comment:12

See #15659.

@jdemeyer
Copy link
Author

comment:13

Replying to @roed314:

More generally, are there other places in Sage where we should be more aggressive about increasing the Pari stack size?

Yes, see #15660.

@roed314
Copy link
Contributor

roed314 commented Jan 10, 2014

comment:14

Cool. I'm doctesting this ticket and will then give it a positive review.

@roed314
Copy link
Contributor

roed314 commented Jan 12, 2014

Reviewer: David Roe

@roed314
Copy link
Contributor

roed314 commented Jan 12, 2014

comment:15

Looks good.

@vbraun
Copy link
Member

vbraun commented Jan 13, 2014

comment:16

Ok, this is not "critical"

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