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

GF(p) constructor should check primality of p only once #12210

Closed
jdemeyer opened this issue Dec 21, 2011 · 9 comments
Closed

GF(p) constructor should check primality of p only once #12210

jdemeyer opened this issue Dec 21, 2011 · 9 comments

Comments

@jdemeyer
Copy link

In the FiniteField constructor, it is first checked whether the order is a prime power, and then whether it is prime. If the order is prime, this means that the primality of the order will be checked twice.

Depends on #11784

Component: number theory

Keywords: sd35

Author: Jeroen Demeyer

Reviewer: Marco Streng

Merged: sage-5.0.beta1

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

@jdemeyer jdemeyer added this to the sage-4.8 milestone Dec 21, 2011
@jdemeyer jdemeyer changed the title GP(p) constructor should check primality of p only once GF(p) constructor should check primality of p only once Dec 21, 2011
@jdemeyer
Copy link
Author

Attachment: 12210_GF_p_is_prime.patch.gz

@sagetrac-florian
Copy link
Mannequin

sagetrac-florian mannequin commented Dec 21, 2011

comment:2

I'm currently reviewing this

@sagetrac-florian
Copy link
Mannequin

sagetrac-florian mannequin commented Dec 21, 2011

comment:3

For some reason the patch keep failing to apply, I've upgraded sage to 7.4.2 to be sure, but it still doesn't apply. I get

applying /storage/florian/12210_GF_p_is_prime.patch
patching file sage/rings/finite_rings/constructor.py
Hunk #1 FAILED at 288
Hunk #2 FAILED at 345
Hunk #3 FAILED at 381
3 out of 3 hunks FAILED -- saving rejects to file sage/rings/finite_rings/constructor.py.rej
abort: patch failed to apply

I'm assuming I'm doing something wrong or something, so someone else can take the reviewing over while I try to sort out the problem on my machine.

@sagetrac-johanbosman
Copy link
Mannequin

sagetrac-johanbosman mannequin commented Dec 21, 2011

comment:4

I think you have to upgrade to 4.8.alpha4 to apply this.

@jdemeyer
Copy link
Author

comment:5

It is because of #11784, you could try applying that patch first

@jdemeyer
Copy link
Author

Dependencies: #11784

@mstreng
Copy link

mstreng commented Jan 5, 2012

Reviewer: Marco Streng

@mstreng
Copy link

mstreng commented Jan 5, 2012

comment:6

All long tests pass on 4.8.alpha4.

@jdemeyer
Copy link
Author

Merged: sage-5.0.beta1

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