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 Conic doctests #33603

Closed
mstreng opened this issue Mar 30, 2022 · 18 comments
Closed

Fix Conic doctests #33603

mstreng opened this issue Mar 30, 2022 · 18 comments

Comments

@mstreng
Copy link

mstreng commented Mar 30, 2022

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

@mstreng mstreng added this to the sage-9.6 milestone Mar 30, 2022
@mstreng
Copy link
Author

mstreng commented Mar 30, 2022

Branch: u/mstreng/fix_conic_doctests

@mstreng
Copy link
Author

mstreng commented Mar 30, 2022

Changed keywords from none to conic, magma

@mstreng
Copy link
Author

mstreng commented Mar 30, 2022

Author: Marco Streng

@mstreng

This comment has been minimized.

@mstreng
Copy link
Author

mstreng commented Mar 30, 2022

New commits:

af11c80Doctest fixes for Conics

@mstreng
Copy link
Author

mstreng commented Mar 30, 2022

Commit: af11c80

@mstreng

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 30, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

714d21aInstead of removing the incorrect cache check, copy the correct cache check from conics over QQ
b991973Add non-Magma example that fails on [SageMath](../wiki/SageMath) 9.5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 30, 2022

Changed commit from af11c80 to b991973

@mstreng

This comment has been minimized.

@mstreng
Copy link
Author

mstreng commented Mar 31, 2022

comment:6

Apparently the cache bug was introduced in #28900.

@tscrim
Copy link
Collaborator

tscrim commented Mar 31, 2022

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Mar 31, 2022

comment:7

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 31, 2022

Branch pushed to git repo; I updated commit sha1. New commits:

8f12947Add comment explaining why we check against a list.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 31, 2022

Changed commit from b991973 to 8f12947

@mstreng
Copy link
Author

mstreng commented Mar 31, 2022

comment:9

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.

@tscrim
Copy link
Collaborator

tscrim commented Mar 31, 2022

comment:10

Thank you. LGTM.

@vbraun
Copy link
Member

vbraun commented Apr 2, 2022

Changed branch from u/mstreng/fix_conic_doctests to 8f12947

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