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

fix bugs in is_prime #5496

Closed
williamstein opened this issue Mar 12, 2009 · 18 comments
Closed

fix bugs in is_prime #5496

williamstein opened this issue Mar 12, 2009 · 18 comments

Comments

@williamstein
Copy link
Contributor

This is not good:

sage: is_prime(GF(5)(3))
True
sage: is_prime(GF(5)(4))
False

The fix is to totally 100% rewrite is_prime in arith.py so that it first calls x.is_prime() and if that isn't defined, then in some special cases (e.g., python ints) converts to Integer and calls is_prime. Otherwise, it raises a NotImplementedError.

Component: number theory

Author: Kevin Stueve

Reviewer: Sebastian Pancratz

Merged: sage-4.3.1.rc1

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

@williamstein williamstein added this to the sage-4.3.1 milestone Mar 12, 2009
@williamstein williamstein self-assigned this Mar 12, 2009
@sagetrac-kevin-stueve
Copy link
Mannequin

sagetrac-kevin-stueve mannequin commented Jan 17, 2010

comment:1

Right now finite fields don't seem to have an is_prime function, so I believe that currently, is_prime(GF(5)(3)) should raise NotImplementedError. I'm going to try to fix is_prime so that it raises NotImplementedError for is_prime(GF(5)(3)).

Kevin Stueve

@sagetrac-kevin-stueve
Copy link
Mannequin

sagetrac-kevin-stueve mannequin commented Jan 17, 2010

Attachment: 5496.patch.gz

changed delegation of is_prime calculation to n.is_prime()

@sagetrac-kevin-stueve sagetrac-kevin-stueve mannequin changed the title fix bugs in is_prime (EASY) fix bugs in is_prime Jan 17, 2010
@sagetrac-kevin-stueve
Copy link
Mannequin

sagetrac-kevin-stueve mannequin commented Jan 17, 2010

Attachment: 5496.2.patch.gz

@sagetrac-kevin-stueve
Copy link
Mannequin

sagetrac-kevin-stueve mannequin commented Jan 17, 2010

comment:4

Apply only 5496.2.patch.

@sagetrac-spancratz
Copy link
Mannequin

sagetrac-spancratz mannequin commented Jan 18, 2010

Three small changes throughout the Sage library

@sagetrac-spancratz
Copy link
Mannequin

sagetrac-spancratz mannequin commented Jan 18, 2010

Attachment: trac5496_rationals_to_int.patch.gz

Attachment: trac5496_symbolic_expressions.patch.gz

Second addendum, for symbolic expressions

@sagetrac-spancratz
Copy link
Mannequin

sagetrac-spancratz mannequin commented Jan 18, 2010

Reviewer: spancratz

@sagetrac-spancratz
Copy link
Mannequin

sagetrac-spancratz mannequin commented Jan 18, 2010

comment:5

Applying Kevin's second patch 5496.2.patch and the two small changes I added, this now passes all doctests done with sage -t devel/sage/sage, and can get a positive review.

@sagetrac-spancratz
Copy link
Mannequin

sagetrac-spancratz mannequin commented Jan 18, 2010

Author: kstueve

@williamstein
Copy link
Contributor Author

comment:6
if type(n) == int or type(n)==long: 

should be

if isinstance(n, (int, long)):

@williamstein
Copy link
Contributor Author

comment:7

Also, use obj.pyobject() in some cases for symbolics...

@sagetrac-spancratz
Copy link
Mannequin

sagetrac-spancratz mannequin commented Jan 18, 2010

Third addendum, for one character change for lucas numbers

@sagetrac-spancratz
Copy link
Mannequin

sagetrac-spancratz mannequin commented Jan 18, 2010

comment:8

Attachment: trac5496_lucas.patch.gz

I've now incorporated the handling of symbolic expressions as suggested by Burcin. The sequence of patches should be applied in the order

  • 5496.2.patch
  • trac5496_rationals_to_int.patch
  • trac5496_symbolic_expressions.patch
  • trac5496_lucas.patch
  • trac5496_symbolic_expressions2.patch

I am running doctests now, but if they pass this should get positive review again.

@sagetrac-spancratz
Copy link
Mannequin

sagetrac-spancratz mannequin commented Jan 18, 2010

Attachment: trac5496_symbolic_expressions2.patch.gz

Fourth addendum, for symbolic expressions

@sagetrac-spancratz
Copy link
Mannequin

sagetrac-spancratz mannequin commented Jan 18, 2010

comment:9

This is to confirm that all doctests have been passed, checked with "./sage -t devel/sage/sage".

@rlmill
Copy link
Mannequin

rlmill mannequin commented Jan 19, 2010

Changed author from kstueve to Kevin Stueve

@rlmill
Copy link
Mannequin

rlmill mannequin commented Jan 19, 2010

Changed reviewer from spancratz to Sebastian Pancratz

@rlmill
Copy link
Mannequin

rlmill mannequin commented Jan 19, 2010

Merged: sage-4.3.1.rc1

@rlmill rlmill mannequin removed the s: positive review label Jan 19, 2010
@rlmill rlmill mannequin closed this as completed Jan 19, 2010
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

1 participant