Fix Conic doctests
Closed this issue · 18 comments
The following optional Magma doctest fails:
File "src/sage/schemes/plane_conics/con_field.py", line 1073, in sage.schemes.plane_conics.con_field.ProjectiveConic_field.rational_point
Failed example:
Conic([L.gen(), 30, -21]).has_rational_point(algorithm='magma') # optional - magma
Expected:
False
Got:
True
This is due to the following bug in Conics over number fields: if the cache of a conic is empty and the user asks whether a rational point exists without asking for the point itself, then has_rational_point always returns True. Here is a non-Magma example documenting this bug:
sage: P.<a> = QuadraticField(2)
sage: C = Conic(P,[1,1,1])
sage: C.has_rational_point()
True
sage: C.has_rational_point(point=True)
(False, None)
sage: C.has_rational_point()
True
sage: C.has_rational_point(obstruction=True)
(True, None)
sage: C.has_rational_point(point=True, obstruction=True)
(False,
Ring morphism:
From: Number Field in a with defining polynomial x^2 - 2 with a = 1.414213562373095?
To: Algebraic Real Field
Defn: a |--> -1.414213562373095?)
sage: C.has_rational_point()
False
Moreover, the documentation incorrectly claims (see #31892)
- that the inverse maps of conic parametrization are incorrect and
- that the
is_onemethod is a good way to check this.
CC: @kliem @JohnCremona @fchapoton @tscrim
Component: geometry
Keywords: conic, magma
Author: Marco Streng
Branch/Commit: 8f12947
Reviewer: Travis Scrimshaw
Issue created by migration from https://trac.sagemath.org/ticket/33603
Branch: u/mstreng/fix_conic_doctests
Changed keywords from none to conic, magma
Author: Marco Streng
Description changed:
---
+++
@@ -1 +1,17 @@
+The following optional Magma doctest fails:
+```
+File "src/sage/schemes/plane_conics/con_field.py", line 1073, in sage.schemes.plane_conics.con_field.ProjectiveConic_field.rational_point
+Failed example:
+ Conic([L.gen(), 30, -21]).has_rational_point(algorithm='magma') # optional - magma
+Expected:
+ False
+Got:
+ True
+```
+This is due to the following bug in Conics over number fields: if the cache of a conic is empty and the user asks whether a rational point exists without asking for the point itself, then `has_rational_point` always returns `True`.
+
+Moreover, the documentation incorrectly claims (see [trac:31892])
+* that the inverse maps of conic parametrization are incorrect and
+* that the `is_one`method is a good way to check this.
+Description changed:
---
+++
@@ -9,7 +9,28 @@
Got:
True
```
-This is due to the following bug in Conics over number fields: if the cache of a conic is empty and the user asks whether a rational point exists without asking for the point itself, then `has_rational_point` always returns `True`.
+This is due to the following bug in Conics over number fields: if the cache of a conic is empty and the user asks whether a rational point exists without asking for the point itself, then `has_rational_point` always returns `True`. Here is a non-Magma example documenting this bug:
+
+```
+sage: P.<a> = QuadraticField(2)
+sage: C = Conic(P,[1,1,1])
+sage: C.has_rational_point()
+True
+sage: C.has_rational_point(point=True)
+(False, None)
+sage: C.has_rational_point()
+True
+sage: C.has_rational_point(obstruction=True)
+(True, None)
+sage: C.has_rational_point(point=True, obstruction=True)
+(False,
+ Ring morphism:
+ From: Number Field in a with defining polynomial x^2 - 2 with a = 1.414213562373095?
+ To: Algebraic Real Field
+ Defn: a |--> -1.414213562373095?)
+sage: C.has_rational_point()
+False
+```
Moreover, the documentation incorrectly claims (see [trac:31892])
* that the inverse maps of conic parametrization are incorrect andDescription changed:
---
+++
@@ -32,7 +32,7 @@
False
```
-Moreover, the documentation incorrectly claims (see [trac:31892])
+Moreover, the documentation incorrectly claims (see #31892)
* that the inverse maps of conic parametrization are incorrect and
* that the `is_one`method is a good way to check this.
Reviewer: Travis Scrimshaw
I guess this comes from _finite_obstructions and _infinite_obstructions being None, which carries a different meaning. Could you add a comment saying something like, "These values might be None, so we explicitly check against a list"? Otherwise if doctests pass (I am assuming this is ready for review; if so, please set it to needs review), then positive review.
Branch pushed to git repo; I updated commit sha1. New commits:
8f12947 | Add comment explaining why we check against a list. |
All (long) non-optional tests passed. All Magma tests in src/sage/schemes/plane_conics passed. And then I added the comment that Travis asked for.
Thank you. LGTM.
Changed branch from u/mstreng/fix_conic_doctests to 8f12947