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

Conic parametrization broken #31892

Closed
kliem opened this issue Jun 1, 2021 · 52 comments
Closed

Conic parametrization broken #31892

kliem opened this issue Jun 1, 2021 · 52 comments

Comments

@kliem
Copy link
Contributor

kliem commented Jun 1, 2021

The final output here is wrong:

sage: c = Conic(GF(2), [1,1,1,1,1,0])
....: 
sage: c.parametrization()
(Scheme morphism:
   From: Projective Space of dimension 1 over Finite Field of size 2
   To:   Projective Conic Curve over Finite Field of size 2 defined by x^2 + x*y + y^2 + x*z + y*z
   Defn: Defined on coordinates by sending (x : y) to
         (x*y + y^2 : x^2 + x*y : x^2 + x*y + y^2),
 Scheme morphism:
   From: Projective Conic Curve over Finite Field of size 2 defined by x^2 + x*y + y^2 + x*z + y*z
   To:   Projective Space of dimension 1 over Finite Field of size 2
   Defn: Defined on coordinates by sending (x : y : z) to
         (y : x))
sage: f, g = c.parametrization()
sage: (g*f).is_one()
False

The same here:

sage: R.<x,y,z> = QQ[]                                                                                                                                                              
sage: C = Curve(7*x^2 + 2*y*z + z^2)                                                                                                                                                
sage: f, g = C.parametrization(); f,g                                                                                                                                               
(Scheme morphism:
   From: Projective Space of dimension 1 over Rational Field
   To:   Projective Conic Curve over Rational Field defined by 7*x^2 + 2*y*z + z^2
   Defn: Defined on coordinates by sending (x : y) to
         (-2*x*y : x^2 + 7*y^2 : -2*x^2),
 Scheme morphism:
   From: Projective Conic Curve over Rational Field defined by 7*x^2 + 2*y*z + z^2
   To:   Projective Space of dimension 1 over Rational Field
   Defn: Defined on coordinates by sending (x : y : z) to
         (-1/2*x : 1/7*y + 1/14*z))
sage: (g*f).is_one()                                                                                                                                                                
False
sage: g([0, -1, 2])                                                                                                                                                                 
...
ValueError: [0, 0] does not define a valid point since all entries are 0
sage: p = g.domain().defining_polynomial()                                                                                                                                          
sage: p([0, -1, 2])                                                                                                                                                                 
0

Depends on #33953

CC: @mstreng @JohnCremona

Component: algebraic geometry

Author: Kwankyu Lee

Branch/Commit: b2af690

Reviewer: Marco Streng

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

@kliem kliem added this to the sage-9.4 milestone Jun 1, 2021
@JohnCremona
Copy link
Member

comment:2

There is an actual mathematical issue here. The map from P^2 to the conic is defined everywhere by a single triple of polynomials, but the map from the conic to P^1 cannot be defined by a single pair.

For example the familiar easiest example X^2 + Y^2 = Z^2

is parametrized by mapping (u : v) to

(x : y : z) = (u^2 - v^2 : 2 u v : u^2 + v^2),

but the reverse map is given by mapping (x : y : z) to

(u : v) = (x + z : y) = (y : z - x)

where

  • the first formula (x + z : y) is defined away from (-1 : 0 : 1)
  • and the second (y : z - x) away from (1 : 0 : 1),
  • and where they are both defined they agree.

That's the way maps between curves works.

Maps betwee (Sage) Schemes should be capable of being defined on patches like this.

The docstring for the method parametrization() does have the

Warning "The second map is currently broken
and neither the inverse nor well-defined."

which while not being grammatical does give a warning.

@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Aug 22, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 18, 2021
@mstreng
Copy link

mstreng commented Mar 29, 2022

comment:5

The inverse is correct in both examples at this ticket description, but the way it works in SageMath is a little bit broken.

Here's what I get in version 9.5. I'm using paramatrization(P) in order to replicate the ticket description's example, because parametrization is randomized if you don't specify a point.

sage: C = Conic(GF(2), [1,1,1,1,1,0])                                           
sage: P = C([0,0,1])                                                            
sage: (f, g) = C.parametrization(P); (f, g)                                     
(Scheme morphism:
   From: Projective Space of dimension 1 over Finite Field of size 2
   To:   Projective Conic Curve over Finite Field of size 2 defined by x^2 + x*y + y^2 + x*z + y*z
   Defn: Defined on coordinates by sending (x : y) to
         (x*y + y^2 : x^2 + x*y : x^2 + x*y + y^2),
 Scheme morphism:
   From: Projective Conic Curve over Finite Field of size 2 defined by x^2 + x*y + y^2 + x*z + y*z
   To:   Projective Space of dimension 1 over Finite Field of size 2
   Defn: Defined on coordinates by sending (x : y : z) to
         (y : x))
sage: g.representatives()                                                                       
[Scheme morphism:
   From: Projective Conic Curve over Finite Field of size 2 defined by x^2 + x*y + y^2 + x*z + y*z
   To:   Projective Space of dimension 1 over Finite Field of size 2
   Defn: Defined on coordinates by sending (x : y : z) to
         (y : x),
 Scheme morphism:
   From: Projective Conic Curve over Finite Field of size 2 defined by x^2 + x*y + y^2 + x*z + y*z
   To:   Projective Space of dimension 1 over Finite Field of size 2
   Defn: Defined on coordinates by sending (x : y : z) to
         (x + y + z : y + z)]
sage: g*f                                                                       
Scheme endomorphism of Projective Space of dimension 1 over Finite Field of size 2
  Defn: Defined on coordinates by sending (x : y) to
        (x^2 + x*y : x*y + y^2)
sage: f*g                                                                       
Scheme endomorphism of Projective Conic Curve over Finite Field of size 2 defined by x^2 + x*y + y^2 + x*z + y*z
  Defn: Defined on coordinates by sending (x : y : z) to
        (x^2 + x*y : x*y + y^2 : x^2 + x*y + y^2)
sage: P1 = f.domain()
sage: P1.hom(P1.gens(), P1) == g*f                             
True

You can see from the output of representatives that g really knows that the first formula is not defined at all points and hence there is a second formula stored inside g, which does work in (0:0:1). This agrees with what John explained. Moreover, we have (x^2+xy : xy + y^2) = (x(x+y) : y(x+y)) = (x : y), so the output of g*f really is the identity map, and the same holds for f*g. So this is correct so far. Then the following is all incorrect:

g(P)
ValueError: [0, 0] does not define a valid point since all entries are 0
(g*f).is_one()
False
(f*g).is_one()
False
sage: C.Hom(C).one() == f*g
False
sage: (x,y,z) = C.gens(); C.hom([x,y,z],C) == f*g                                                   
False

Something indeed needs to be done about this. The following indicates that we are very close.

sage: g.representatives()[1](P)
(1 : 1)
sage: (f*g).representatives()[0]                                                                
Scheme endomorphism of Projective Conic Curve over Finite Field of size 2 defined by x^2 + x*y + y^2 + x*z + y*z
  Defn: Defined on coordinates by sending (x : y : z) to
        (x : y : z)

Here are some other things that don't work:

sage: (f*g).representatives()[0].is_one()
False
sage: C.Hom(C).one().is_one()
TypeError: ...
sage: (g*f).representatives()                                                                   
AttributeError: ...

But I don't know enough about the inner workings of SageMath schemes to fix it.

ps. The warning mentioned by John refers to this ticket, so there seems to be no reason to doubt the output of parametrization.

@mstreng

This comment has been minimized.

@mstreng

This comment has been minimized.

@mstreng
Copy link

mstreng commented Mar 30, 2022

comment:7

See #33603: Fix Conic doctests.

@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 May 3, 2022
@kwankyu
Copy link
Collaborator

kwankyu commented Jun 5, 2022

Branch: u/klee/31892

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 5, 2022

Author: Kwankyu Lee

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2022

Commit: 9b0ba8f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2022

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

9b0ba8fRefactor identity morphism

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 5, 2022

comment:11

Part of the issue is fixed incidentally by #33953.

The branch fixes other issues related with the identity morphisms.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2022

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

7e9b7d3Fix some doctests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2022

Changed commit from 9b0ba8f to 7e9b7d3

@kwankyu

This comment has been minimized.

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 5, 2022

comment:14

With #33953, this g([0, -1, 2]) gives the correct answer.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2022

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

f89842eFix more doctest errors

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2022

Changed commit from 7e9b7d3 to f89842e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2022

Changed commit from f89842e to 78ba3a9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2022

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

78ba3a9Fix morphism of conics

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 6, 2022

comment:17

The last commit contains fixes so that g([0, -1, 2]) gives the correct answer.

@kwankyu kwankyu added this to the sage-9.8 milestone Sep 1, 2022
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 1, 2022

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

d9982f3Extend to morphisms to affine spaces

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 1, 2022

Changed commit from c95d429 to d9982f3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 1, 2022

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

e66bb26Minor edits

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 1, 2022

Changed commit from d9982f3 to e66bb26

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 1, 2022

comment:33

Added Cremona's example :) Needs review.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 11, 2022

Changed commit from e66bb26 to 527c3a4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 11, 2022

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

527c3a4Merge branch 'develop' into t/31892/31892

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 11, 2022

comment:35

Rebased onto the latest develop branch, with the side purpose of checking trac after recent maintenance.

@mstreng
Copy link

mstreng commented Oct 14, 2022

comment:36

Tests pass. Code looks good. Just one thing before giving this a positive review:

Could you change the following mathematically incorrect example?

sage: C = Curve(x^2 + y^2 - z^2)
sage: A.<u> = AffineSpace(QQ, 1)
sage: f = C.hom([(x + z)/y], A)
sage: g = C.hom([y/(z - x)], A)
sage: f == g
True

The reason: it relies on mathematically incorrect behaviour of SageMath. There are no non-constant morphisms from C to A. Indeed, all non-constant morphisms from C to P1 are surjective, hence do not land inside A. For example, f((1:0:1)) is not defined as an element of A. SageMath incorrectly claims that f is a morphism, as follows:

sage: R.<x,y,z> = QQ[]                                                                                                      
sage: C = Curve(x^2 + y^2 - z^2)                                                                                            
sage: A.<u> = AffineSpace(QQ, 1)                                                                                            
sage: f = C.hom([(x + z)/y], A)                                                                                             
sage: f                                                                                                                     
Scheme morphism:
  From: Projective Conic Curve over Rational Field defined by x^2 + y^2 - z^2
  To:   Affine Space of dimension 1 over Rational Field
  Defn: Defined on coordinates by sending (x : y : z) to
        ((x + z)/y)
sage: f.parent()                                                                                                            
Set of morphisms
  From: Projective Conic Curve over Rational Field defined by x^2 + y^2 - z^2
  To:   Affine Space of dimension 1 over Rational Field

This behaviour is not introduced at this ticket, but was already there in e.g. SageMath 9.5, so you do not need to fix it. But we should not add examples to the documentation that rely on this incorrect behaviour.
Perhaps you could change the example as follows:

sage: C = Curve(x^2 + y^2 - z^2)
sage: P.<u,v> = ProjectiveSpace(QQ, 1)
sage: f = C.hom([x + z, y], P)
sage: g = C.hom([y, z - x], P)
sage: f == g
True

And/or you could give constant examples with codomain A. And perhaps you could also add an h for which f == h is False.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 14, 2022

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

c789509Merge branch 'develop' into t/31892/31892
b2af690Fix an errorneous example

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 14, 2022

Changed commit from 527c3a4 to b2af690

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 14, 2022

comment:38

Replying to Marco Streng:

Could you change the following mathematically incorrect example?

sage: C = Curve(x^2 + y^2 - z^2)
sage: A.<u> = AffineSpace(QQ, 1)
sage: f = C.hom([(x + z)/y], A)
sage: g = C.hom([y/(z - x)], A)
sage: f == g
True

The reason: it relies on mathematically incorrect behaviour of SageMath. There are no non-constant morphisms from C to A. Indeed, all non-constant morphisms from C to P1 are surjective, hence do not land inside A. For example, f((1:0:1)) is not defined as an element of A.

You are right. I wonder how I transformed John's correct example to a wrong one!

Perhaps you could change the example as follows:

sage: C = Curve(x^2 + y^2 - z^2)
sage: P.<u,v> = ProjectiveSpace(QQ, 1)
sage: f = C.hom([x + z, y], P)
sage: g = C.hom([y, z - x], P)
sage: f == g
True

and perhaps you could also add an h for which f == h is False.

I did this. Thank you.

@JohnCremona
Copy link
Member

comment:39

Thanks to all (both) of you for working on this.

@mstreng
Copy link

mstreng commented Oct 20, 2022

comment:40

Replying to Marco Streng:

Just one thing before giving this a positive review

Thanks for fixing this. Looks good to me.

@mstreng
Copy link

mstreng commented Oct 20, 2022

Reviewer: Marco Streng

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 21, 2022

comment:42

Thanks for the review!

@vbraun
Copy link
Member

vbraun commented Oct 23, 2022

Changed branch from u/klee/31892 to b2af690

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

7 participants