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

projective space homs do not check arguments sufficiently #3964

Closed
JohnCremona opened this issue Aug 27, 2008 · 14 comments
Closed

projective space homs do not check arguments sufficiently #3964

JohnCremona opened this issue Aug 27, 2008 · 14 comments

Comments

@JohnCremona
Copy link
Member

Alex Ghitza reported:

I am fairly certain the following two things are bugs, but I want to
double-check that I'm not doing something stupid before submitting a ticket:

sage: R.<x,y> = QQ[]
sage: P1 = ProjectiveSpace(R)
sage: H = P1.Hom(P1)
sage: f = H([x-y, x*y])
sage: f

Scheme endomorphism of Projective Space of dimension 1 over Rational Field
 Defn: Defined on coordinates by sending (x : y) to
       (x - y : x*y)


This is nonsense: there is no morphism from P1 to P1 given by those
equations, since the two polynomials x-y and x*y are not homogeneous of
the same degree.  I think Sage should throw a ValueError here.

The second example:

sage: R.<x,y> = QQ[]
sage: P1 = ProjectiveSpace(R)
sage: H = P1.Hom(P1)
sage: f = H([x^2, x*y])
sage: f

Scheme endomorphism of Projective Space of dimension 1 over Rational Field
 Defn: Defined on coordinates by sending (x : y) to
       (x^2 : x*y)


This is also bad: the two polynomials are now homogeneous of degree 2,
but they are not relatively prime (and so this is not a morphism from P1
to P1, but rather a rational map since it is not defined at (0 : y)).  I
think Sage should also throw a ValueError here.

(Or maybe I'm doing things wrong, in which case I'd love to find out how
to make this work.)

to which John Cremona added:

You are definitely right.  The problem lies (as far as I can see) in
sage.schemes.generic in the __init__ funtion of class
SchemeMorphism_on_points_projective_space.  (I only found this out by
tring to construct a morphism from P^1 to P^1 using 3 polynomials,
which did raise an error in this very function.)

It appears that the only check this function does is that the number
of polys is correct.  It does not check that they are actually polys,
or have the right number of variables, let alone that they are coprime
and homogeneous of the same degree:

sage: S.<u,v,w> = QQ[]
sage: f = H([u,v])
sage: f = H([u*v*w,u+v+w])
sage: f = H([exp(u),exp(v)])
sage: f

Scheme endomorphism of Projective Space of dimension 1 over Rational Field
 Defn: Defined on coordinates by sending (x : y) to
       (e^u : e^v)

with H as in your example.

This definitely deserves a ticket, which I will create. now.

Component: algebraic geometry

Keywords: projective space morphism

Author: Alex Ghitza, William Stein

Reviewer: John Cremona

Merged: sage-4.3.1.rc1

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

@aghitza
Copy link

aghitza commented Dec 21, 2008

comment:2

I finally got around to fixing this. The attached patch fixes the issues reported above (by John and myself), and adds some doctests.

@aghitza
Copy link

aghitza commented Dec 21, 2008

Attachment: trac_3964.patch.gz

@JohnCremona
Copy link
Member Author

comment:3

This looks pretty good (I have not actually tested it yet though). Alex, can we not check that the polys define a map to the correct space by checking that the defining polys of the target (if any) vanish when evaluated at the tuple of polys defining the map? If that works it would be worthwhile adding it.

@aghitza
Copy link

aghitza commented Dec 21, 2008

comment:4

Good point. I will implement this and submit a new patch. I also just realized that I misread a docstring and implemented some things the wrong way around, so I will get that fixed.

@aghitza aghitza changed the title projective space homs do not check arguments sufficiently [not ready for review] projective space homs do not check arguments sufficiently Dec 21, 2008
@aghitza aghitza assigned aghitza and unassigned williamstein Mar 29, 2009
@aghitza

This comment has been minimized.

@williamstein
Copy link
Contributor

apply this (not the previous one).

@williamstein
Copy link
Contributor

comment:7

Attachment: trac_3964-rebased_and_extended.patch.gz

@williamstein
Copy link
Contributor

comment:8

Hi,

I took Alex's patch, added a bunch of doctests, changed it some, and read it. I think this should go in if it looks ok to John or Alex. It's a clear improvement, though arbitrarily much works remains!

@JohnCremona
Copy link
Member Author

Author: Alex Ghitza, William Stein

@JohnCremona
Copy link
Member Author

comment:9

Patch applies fine to 4.3.1.rc0 and tests pass. So I'll give this a positive review despite the fact that (as was said) there's a lot more needing to be done, for example:

sage: S.<u,v,w> = QQ[]
sage: C = Curve(u^2+v^2-w^2); C
Projective Curve over Rational Field defined by u^2 + v^2 - w^2
sage: H = C.Hom(C); H
Set of points of Projective Curve over Rational Field defined by u^2 + v^2 - w^2 defined over Quotient of Multivariate Polynomial Ring in u, v, w over Rational Field by the ideal (u^2 + v^2 - w^2)
sage: 
sage: H([u,v,w])
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
...
AttributeError: 'QuotientRingElement' object has no attribute 'degree'

@JohnCremona
Copy link
Member Author

Reviewer: John Cremona

@JohnCremona JohnCremona changed the title [not ready for review] projective space homs do not check arguments sufficiently projective space homs do not check arguments sufficiently Jan 17, 2010
@rlmill
Copy link
Mannequin

rlmill mannequin commented Jan 18, 2010

Merged: age-4.3.1.rc1

@rlmill rlmill mannequin removed the s: positive review label Jan 18, 2010
@rlmill rlmill mannequin closed this as completed Jan 18, 2010
@rlmill
Copy link
Mannequin

rlmill mannequin commented Jan 18, 2010

Changed merged from age-4.3.1.rc1 to sage-4.3.1.rc1

@JohnCremona
Copy link
Member Author

comment:12

See #10297 for the next step in this direction.

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