Opened 13 years ago
Closed 7 years ago
#6881 closed enhancement (fixed)
Solving conics over polynomial rings.
Reported by:  Victor S. Miller  Owned by:  Lennart Ackermans 

Priority:  major  Milestone:  sage6.10 
Component:  algebraic geometry  Keywords:  conic, curve, function field 
Cc:  Marco Streng  Merged in:  
Authors:  Lennart Ackermans  Reviewers:  Marco Streng 
Report Upstream:  N/A  Work issues:  
Branch:  995229c (Commits, GitHub, GitLab)  Commit:  995229c89fd78d2f4f0782b1983537b4790a8222 
Dependencies:  Stopgaps: 
Description (last modified by )
Implement the algorithm of John Cremona and Mark van Hoeij for solving conics over fraction fields of polynomial rings.
The article http://www.warwick.ac.uk/~masgaj/papers/conicFT.pdf contains references to implementations in Maple (van Hoeij) and Magma (CremonaRoberts), but these implementations heavily rely on things specific to these systems.
#727 provides classes for conic curves that this code should build upon
Change History (36)
comment:1 Changed 13 years ago by
Component:  algebra → algebraic geometry 

comment:2 Changed 12 years ago by
Report Upstream:  → N/A 

comment:3 Changed 12 years ago by
Cc:  Marco Streng added 

comment:4 Changed 12 years ago by
Description:  modified (diff) 

Summary:  Conics → Solving conics over polynomial rings. 
I changed the description to better fit what is already in #727. Besides things that are already in #727, all that I removed from the original description were the following two requests.
 Use John Cremona's algorithms for finding points on conics over QQ.
It seems that Simon's algorithms (in #727) are better, but that doesn't have to stop us from giving Cremona's code as an option. It is inside mwrank, which is part of Sage. If someone wants to do it, then it can be made into a separate ticket.
 Getting primes of bad reduction of conics.
This is as good as in #727: make a Conic C. Then do C.determinant().factor()
comment:5 Changed 7 years ago by
Owner:  changed from tbd to Lennart Ackermans 

comment:6 Changed 7 years ago by
Branch:  → public/conics_rational_function_field 

comment:7 Changed 7 years ago by
Commit:  → 10cfe65b0ea45af8603d3a48aede6c2b6aa55cda 

Branch pushed to git repo; I updated commit sha1. New commits:
10cfe65  Continuing has_rational_point() on conics over rational function fields. Currently returns incorrect solution.

comment:8 Changed 7 years ago by
Commit:  10cfe65b0ea45af8603d3a48aede6c2b6aa55cda → 73b7b05dfd2584ccb29964310d3241aee1d1f82b 

Branch pushed to git repo; I updated commit sha1. New commits:
73b7b05  Fixed has_rational_point and added some documentation

comment:9 Changed 7 years ago by
Commit:  73b7b05dfd2584ccb29964310d3241aee1d1f82b → 8f4d0f46e9a4b021e0d113bf80c636f2be038d2a 

Branch pushed to git repo; I updated commit sha1. New commits:
8f4d0f4  Added documentation and bugfixes to con_rational_function_field

comment:10 Changed 7 years ago by
Authors:  Victor Miller → Lennart Ackermans 

comment:11 Changed 7 years ago by
Status:  new → needs_review 

comment:12 Changed 7 years ago by
Commit:  8f4d0f46e9a4b021e0d113bf80c636f2be038d2a → 8a9d34fd202d381912a41e7b189fad21b0126408 

Branch pushed to git repo; I updated commit sha1. New commits:
8a9d34f  Fixed documentation bug

comment:13 Changed 7 years ago by
Description:  modified (diff) 

Keywords:  function field added 
Reviewers:  → Marco Streng 
comment:14 Changed 7 years ago by
Description:  modified (diff) 

comment:15 Changed 7 years ago by
Milestone:  sagefeature → sage6.10 

Priority:  minor → major 
comment:16 followup: 18 Changed 7 years ago by
Status:  needs_review → needs_work 

Not 100% doctest coverage, and there is a doctest that fails.
comment:17 Changed 7 years ago by
Commit:  8a9d34fd202d381912a41e7b189fad21b0126408 → c6d8368fa1c7ccb579c9f430da111f4fd09c2069 

comment:18 Changed 7 years ago by
Status:  needs_work → needs_review 

Replying to mstreng:
Not 100% doctest coverage, and there is a doctest that fails.
Should be okay now.
comment:19 Changed 7 years ago by
Commit:  c6d8368fa1c7ccb579c9f430da111f4fd09c2069 → b153ba50e28d8b7f744ee469f22af0027888f496 

Branch pushed to git repo; I updated commit sha1. New commits:
b153ba5  Improvements and bug fixes

comment:20 Changed 7 years ago by
Status:  needs_review → needs_work 

comment:21 Changed 7 years ago by
Commit:  b153ba50e28d8b7f744ee469f22af0027888f496 → 49ecc851a3fc2c3f60ae84984426ac61bf8c6db4 

Branch pushed to git repo; I updated commit sha1. New commits:
49ecc85  Some small adjustments

comment:22 Changed 7 years ago by
Status:  needs_work → needs_review 

comment:23 Changed 7 years ago by
Commit:  49ecc851a3fc2c3f60ae84984426ac61bf8c6db4 → ee31fca60f6045b675dde0186f84f8eb983d8bf6 

Branch pushed to git repo; I updated commit sha1. New commits:
ee31fca  Some small adjustments (2)

comment:24 Changed 7 years ago by
Status:  needs_review → positive_review 

All tests pass. Documentation looks good.
The functions do not work perfectly in all cases due to #20003, but after bypassing squarefree_decomposition
, I get:
sage: K.<t> = PolynomialRing(GF(7)) sage: C = Conic([5*t^2+4, t^2+3*t+3, 6*t^2+3*t+2, 5*t^2+5, 4*t+3, 4*t^2+t+5]) sage: C.has_rational_point() True
and
sage: F = FiniteField(7) sage: P.<t> = F[] sage: K = P.fraction_field() sage: for i in range(50): c = [K.random_element() for j in range(6)] C = Conic(c) C.has_rational_point(point=True) ....: (False, None) (False, None) (False, None) (False, None) (True, ((2*t^8 + 5*t^7 + 6*t^6 + 5*t^5 + 4*t^4 + 5*t^2 + 3*t + 2)/(t^8 + 5*t^7 + 5*t^6 + 4*t^5 + 3*t^4 + 2*t^3 + t^2 + 5*t + 6) : (t^8 + 2*t^7 + t^5 + t^4 + 2*t^3 + 2 *t^2 + 4*t + 4)/(t^8 + 6*t^7 + t^6 + 4*t^5 + t^4 + 2*t^2 + 5*t + 3) : 1)) (False, None) (False, None) (False, None) (True, ((2*t^8 + t^7 + 6*t^6 + 4*t^5 + 6*t^4 + 5*t^2 + 1)/(t^8 + 2*t^7 + 6*t^6 + 3*t^5 + 3*t^4 + 6*t^3 + 6*t^2 + 6*t) : (2*t^8 + 4*t^7 + 5*t^6 + 3*t^5 + t^4 + 5*t + 1 )/(t^8 + 2*t^7 + 6*t^6 + 3*t^5 + 3*t^4 + 6*t^3 + 6*t^2 + 6*t) : 1)) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (True, ((2*t^4 + 2*t^3 + 3*t^2)/(t^7 + t^6 + 5*t^4 + t^2 + t + 4) : (5*t^7 + 3*t^6 + t ^5 + 5*t^4 + 6*t^3 + 2*t^2 + 4*t)/(t^7 + t^6 + 5*t^4 + t^2 + t + 4) : 1)) (False, None) (False, None) (False, None) (False, None) (False, None) (True, ((t^8 + 5*t^7 + 2*t^6 + 2*t^5 + 3*t^3 + t^2 + 3)/(t^7 + t^6 + 6*t^4 + 5*t^3 + 6 *t^2 + 2*t + 2) : (4*t^5 + 2*t^4 + 5*t^3 + 4*t^2 + 5*t + 2)/(t^4 + 2*t^3 + 5*t^2 + 4*t + 5) : 1)) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (False, None) (True, ((2*t^11 + 3*t^10 + 6*t^9 + t^8 + 6*t^7 + 4*t^6 + t^5 + 4*t^4 + 2*t^3 + 3*t + 5 )/(t^11 + 5*t^10 + 5*t^9 + t^8 + t^7 + 6*t^6 + 6*t^5 + 4*t^4 + 6*t^3 + 4*t) : (2 *t^9 + 4*t^8 + 4*t^7 + 6*t^6 + 5*t^5 + 5*t^4 + 4*t^3 + t^2 + 6*t + 1)/(t^10 + 5* t^8 + 4*t^7 + 2*t^6 + 3*t^5 + 5*t^4 + 6*t^2 + 5*t) : 1)) (False, None) (False, None) (False, None) (False, None)
comment:25 followup: 31 Changed 7 years ago by
Status:  positive_review → needs_work 

Please rebase to sage7.1.beta2 (in particular, sage.rings.arith
has moved to sage.arith
) and use the standard copyright template for the newly added file.
If you have done this, you can set this ticket back to positive review.
comment:26 Changed 7 years ago by
You might also want to fix these pyflakes
warnings:
src/sage/schemes/plane_conics/con_rational_function_field.py:25: 'NumberField' imported but unused src/sage/schemes/plane_conics/con_rational_function_field.py:26: 'identity_matrix' imported but unused src/sage/schemes/plane_conics/con_rational_function_field.py:32: redefinition of unused 'vector' from line 30
comment:27 Changed 7 years ago by
Commit:  ee31fca60f6045b675dde0186f84f8eb983d8bf6 → 96c2cc84c16865f36d9833d6745ab8f4c9a995fc 

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
333fe2f  Fixed has_rational_point and added some documentation

038caec  Added documentation and bugfixes to con_rational_function_field

7d0e94a  Documentation update

de731fe  Fixed documentation bug

fbd8543  Improvements and bug fixes

81d6407  Some small adjustments

731d696  Some small adjustments (2)

483349e  Some small adjustments (3)

004758e  Fixed conflict

96c2cc8  Rebase to 7.1.beta2

comment:28 Changed 7 years ago by
Status:  needs_work → positive_review 

comment:29 followup: 32 Changed 7 years ago by
Status:  positive_review → needs_review 

10 new commits? Sorry, but this needs to be reviewed (not by me).
comment:30 Changed 7 years ago by
Status:  needs_review → needs_work 

This is malformatted:
EXAMPLES: Create a conic:: sage: K = FractionField(PolynomialRing(QQ, 't'))
It should be
EXAMPLES: Create a conic:: sage: K = FractionField(PolynomialRing(QQ, 't'))
comment:31 followup: 34 Changed 7 years ago by
Please do this:
Replying to jdemeyer:
use the standard copyright template for the newly added file.
comment:32 Changed 7 years ago by
Replying to jdemeyer:
10 new commits? Sorry, but this needs to be reviewed (not by me).
Only the last 3 are new
comment:33 Changed 7 years ago by
Commit:  96c2cc84c16865f36d9833d6745ab8f4c9a995fc → 995229c89fd78d2f4f0782b1983537b4790a8222 

Branch pushed to git repo; I updated commit sha1. New commits:
995229c  Standard copyright template

comment:34 Changed 7 years ago by
Status:  needs_work → needs_review 

Replying to jdemeyer:
Please do this:
Replying to jdemeyer:
use the standard copyright template for the newly added file.
Sorry, I misunderstood your last comment. All should be okay now.
comment:35 Changed 7 years ago by
Status:  needs_review → positive_review 

Thanks Jeroen. I checked the changes and documentation html, and I ran the doctests again.
comment:36 Changed 7 years ago by
Branch:  public/conics_rational_function_field → 995229c89fd78d2f4f0782b1983537b4790a8222 

Resolution:  → fixed 
Status:  positive_review → closed 
See #727 A patch defining a conic class and using Simon's algorithms for finding points over Q is in progress.