Conic parametrization broken
Closed this issue · 52 comments
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
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.
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.
Description changed:
---
+++
@@ -1,4 +1,4 @@
-`src/sage/schemes/plane_conics/con_field.py` documents mathematically incorrect behavior:
+`src/sage/schemes/plane_conics/con_field.py` documents ~~mathematically~~ incorrect behavior:
```
sage: c = Conic(GF(2), [1,1,1,1,1,0])
@@ -17,17 +17,17 @@
(y : x))
```
-The second map is supposed to be an inverse, however it is not even well-defined (`(0 : 0 : 1)`). The correct inverse should be:
-`Defined on coordinates by sending (x : y : z) to (x + z : y + z)`
+The second map is ~~supposed to be~~ an inverse~~, however it is not even well-defined (`(0 : 0 : 1)`). The correct inverse should be:
+`Defined on coordinates by sending (x : y : z) to (x + z : y + z)`~~
-This is a perfect example how a simple sanity check
+~~This is a perfect example how a simple sanity check~~
```
sage: f, g = c.parametrization()
sage: (g*f).is_one()
...
```
-or similar can prevent things like this.
+~~or similar can prevent things like this.~~
On the other hand, printed out information is not always checked by hand.
Description changed:
---
+++
@@ -29,8 +29,6 @@
```
~~or similar can prevent things like this.~~
-On the other hand, printed out information is not always checked by hand.
-
The bug is not limited to finite fields:
```Branch: u/klee/31892
Author: Kwankyu Lee
Commit: 9b0ba8f
Branch pushed to git repo; I updated commit sha1. New commits:
9b0ba8f | Refactor identity morphism |
Part of the issue is fixed incidentally by #33953.
The branch fixes other issues related with the identity morphisms.
Branch pushed to git repo; I updated commit sha1. New commits:
7e9b7d3 | Fix some doctests |
Description changed:
---
+++
@@ -1,35 +1,24 @@
-`src/sage/schemes/plane_conics/con_field.py` documents ~~mathematically~~ incorrect behavior:
+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))
-```
-
-The second map is ~~supposed to be~~ an inverse~~, however it is not even well-defined (`(0 : 0 : 1)`). The correct inverse should be:
-`Defined on coordinates by sending (x : y : z) to (x + z : y + z)`~~
-
-~~This is a perfect example how a simple sanity check~~
-
-```
+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
```
-~~or similar can prevent things like this.~~
-
-The bug is not limited to finite fields:
+The same here:
```
sage: R.<x,y,z> = QQ[] Branch pushed to git repo; I updated commit sha1. New commits:
f89842e | Fix more doctest errors |
Branch pushed to git repo; I updated commit sha1. New commits:
78ba3a9 | Fix morphism of conics |
The last commit contains fixes so that g([0, -1, 2]) gives the correct answer.
Sorry for my ignorance, but I am unable to merge this ticket on top of SageMath 9.7.beta3 plus #28263 #32340 #33213 #33619 #33840 #33854 #33974 #33978 #33979 #33984 #33985 #33987 #33990 #33991 #33992 #33993 #33996 #34001 #34007 #34008 #34009 #34010 #34014 #34019 #34021 #34022 #34025 #34030 #34034 #34035 #34036 #34037 (all of them positively reviewed):
% git clone -c core.symlinks=true --branch develop --tags https://github.com/sagemath/sage.git
[...]
% cd sage
% git-trac-merge 28263 32340 33213 33619 33840 33854 33974 33978 33979 33984 33985 33987 33990 33991 33992 33993 33996 34001 34007 34008 34009 34010 34014 34019 34021 34022 34025 34030 34034 34035 34036 34037
[...]
% git-trac-merge 31892
#31892
remote branch: u/klee/31892
From git://trac.sagemath.org/sage
* branch u/klee/31892 -> FETCH_HEAD
Auto-merging src/sage/schemes/toric/homset.py
Auto-merging src/sage/schemes/projective/projective_space.py
Auto-merging src/sage/schemes/generic/scheme.py
Auto-merging src/sage/schemes/generic/morphism.py
CONFLICT (content): Merge conflict in src/sage/schemes/generic/morphism.py
Automatic merge failed; fix conflicts and then commit the result.
%
Branch pushed to git repo; I updated commit sha1. New commits:
0bc6d3f | Merge branch 'develop' |
Replying to @GMS103:
Sorry for my ignorance, but I am unable to merge this ticket on top of SageMath 9.7.beta3 plus #28263 #32340 #33213 #33619 #33840 #33854 #33974 #33978 #33979 #33984 #33985 #33987 #33990 #33991 #33992 #33993 #33996 #34001 #34007 #34008 #34009 #34010 #34014 #34019 #34021 #34022 #34025 #34030 #34034 #34035 #34036 #34037 (all of them positively reviewed)
Uploaded the merge with 9.7.beta3. The release manager will decide which of conflicting positively reviewed tickets are merged first to the next beta.
Thanks for explaining.
Branch pushed to git repo; I updated commit sha1. New commits:
39ed047 | Merge branch develop |
I see that #33953 was changed 4 days ago. Does that mean that this ticket needs work as well?
By the way, this ticket fixes (g*f).is_one(), but leaves (f*g).is_one() incorrect. Could something be done about that at the same time, or would that need a separate ticket? With either example from the ticket description, I get
sage: (f*g).is_one()
False
sage: (f*g).representatives()[0]
Scheme endomorphism of Projective Conic Curve over ... defined by ...
Defn: Defined on coordinates by sending (x : y : z) to
(x : y : z)
The first output should be True. The parametrization is correct as shown by the second output.
Replying to @mstreng:
I see that #33953 was changed 4 days ago. Does that mean that this ticket needs work as well?
Yes. There is a merge conflict. I will soon push the merge with #33953.
By the way, this ticket fixes
(g*f).is_one(), but leaves(f*g).is_one()incorrect. Could something be done about that at the same time, or would that need a separate ticket?
Right. (g*f).is_one() works correctly because g*f is a morphism of a projective line, where equality check works correctly. f*g gives wrong result because correct equality check is not done(not implemented) for morphisms of projective subschemes.
I suggest to open a separate ticket to solve the problem of f*g.
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
817e341 | Merge branch 'develop' into t/33953/degree-morphism |
736d448 | Moving the main computation to be done on demand. |
5285e67 | Moving Singular conversion code to libs/singular; converting resoltuion files to Python files. |
5917129 | Changing the doc for clarity and to add some more descriptions. |
3f1d1c3 | Merge branch 'develop' into t/33950/public/rings/free_multigraded_resolutions-33950 |
ab725f2 | Fix spaces |
dabe765 | Merge branch 't/33950/public/rings/free_multigraded_resolutions-33950' into t/33953/degree-morphism |
3947c02 | Refactor conic curves |
ac60efe | Fixes for reviewer comments |
1d7124e | Merge branch 't/33953/degree-morphism' |
Replying to @kwankyu:
I suggest to open a separate ticket to solve the problem of
f*g.
Since this is not a problem of conic parametrization.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c95d429 | Refactor identity morphism |
All fixed.
Branch pushed to git repo; I updated commit sha1. New commits:
d9982f3 | Extend to morphisms to affine spaces |
Branch pushed to git repo; I updated commit sha1. New commits:
e66bb26 | Minor edits |
Added Cremona's example :) Needs review.
Branch pushed to git repo; I updated commit sha1. New commits:
527c3a4 | Merge branch 'develop' into t/31892/31892 |
Rebased onto the latest develop branch, with the side purpose of checking trac after recent maintenance.
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.
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 TrueThe 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 Trueand perhaps you could also add an h for which f == h is False.
I did this. Thank you.
Thanks to all (both) of you for working on this.
Replying to Marco Streng:
Just one thing before giving this a positive review
Thanks for fixing this. Looks good to me.
Reviewer: Marco Streng
Thanks for the review!
Changed branch from u/klee/31892 to b2af690