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

base_ring is wrong for rational points in a projective space over a finite field #34336

Closed
sagetrac-msaaltink mannequin opened this issue Aug 10, 2022 · 14 comments
Closed

Comments

@sagetrac-msaaltink
Copy link
Mannequin

sagetrac-msaaltink mannequin commented Aug 10, 2022

This is wrong:

sage: P3 = ProjectiveSpace(2, GF(3))                                                                                                                                                                                
sage: a = P3( (0,0,1) ); a                                                                                                                                                                                          
(0 : 0 : 1)
sage: a.parent()                                                                                                                                                                                                    
Set of rational points of Projective Space of dimension 2 over Finite Field of size 3
sage: a.base_ring()                                                                                                                                                                                                 
Integer Ring
sage: a.parent().base_ring()                                                                                                                                                                                        
Integer Ring

Surely in both cases the base ring should be GF(3).

This also afflicts points generated in other ways. For example

sage: x,y,z = P3.gens()                                                                                                                                                                                             
sage: C = Curve(x+y+z)                                                                                                                                                                                              
sage: b = C.rational_points()[0]                                                                                                                                                                                    
sage: b                                                                                                                                                                                                             
(0 : 2 : 1)
sage: b.parent()                                                                                                                                                                                                    
Set of rational points of Projective Plane Curve over Finite Field of size 3 defined by x0 + x1 + x2
sage: b.base_ring()                                                                                                                                                                                                 
Integer Ring

Component: algebraic geometry

Author: Lorenz Panny

Branch/Commit: 1626caf

Reviewer: Kwankyu Lee

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

@sagetrac-msaaltink sagetrac-msaaltink mannequin added this to the sage-9.7 milestone Aug 10, 2022
@yyyyx4
Copy link
Member

yyyyx4 commented Aug 14, 2022

Changed author from Mark Saaltink to Lorenz Panny

@yyyyx4
Copy link
Member

yyyyx4 commented Aug 14, 2022

Commit: 6dc2131

@yyyyx4
Copy link
Member

yyyyx4 commented Aug 14, 2022

comment:1

These fairly simple changes seem to do the trick.

(I think the "author" field is for the author of the patch, not the bug report — someone please correct me if I'm wrong!)


New commits:

658c959move .base_ring() up from SchemeMorphism_polynomial to SchemeMorphism
6dc2131automatically determine SchemeMorphism base ring if none is given

@yyyyx4
Copy link
Member

yyyyx4 commented Aug 14, 2022

Branch: public/34336

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 14, 2022

Changed commit from 6dc2131 to cad694c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 14, 2022

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

b07543aMerge branch 'develop' into t/34336/public/34336
cad694cSome edits and import from sage.structure.element

@kwankyu

This comment has been minimized.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 14, 2022

comment:4

I made small edits.

diff --git a/src/sage/schemes/generic/homset.py b/src/sage/schemes/generic/homset.py
index 4115f163b4..5a67c614ef 100644
--- a/src/sage/schemes/generic/homset.py
+++ b/src/sage/schemes/generic/homset.py
@@ -154,7 +154,7 @@ class SchemeHomsetFactory(UniqueFactory):
         if isinstance(Y, CommutativeRing):
             Y = AffineScheme(Y)
         if base is None:
-            from sage.structure.all import coercion_model
+            from sage.structure.element import coercion_model
             base = coercion_model.common_parent(X.base_ring(), Y.base_ring())
         if is_AffineScheme(base):
             base_spec = base

It is not recommended to import from .all, which is intended for filling the global namespace for users.

diff --git a/src/sage/schemes/generic/morphism.py b/src/sage/schemes/generic/morphism.py
index b1409f05ba..4c75777c1d 100644
--- a/src/sage/schemes/generic/morphism.py
+++ b/src/sage/schemes/generic/morphism.py
@@ -482,7 +482,7 @@ class SchemeMorphism(Element):
     def base_ring(self):
         r"""
         Return the base ring of ``self``, that is, the ring over which
-        the coefficients of ``self`` are given as polynomials.
+        the defining polynomials of ``self`` are defined.
 
         OUTPUT:
 

as the meaning of the original sentence is not clear.

I am positive otherwise.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 14, 2022

Reviewer: Kwankyu Lee

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 14, 2022

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

1626cafFix a doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 14, 2022

Changed commit from cad694c to 1626caf

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 14, 2022

comment:7

You can set positive review if you are okay with my edits.

@yyyyx4
Copy link
Member

yyyyx4 commented Aug 14, 2022

comment:8

Looks good, thank you!

@vbraun
Copy link
Member

vbraun commented Aug 30, 2022

Changed branch from public/34336 to 1626caf

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