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

Computing plane curve models for algebraic curves #20790

Closed
sagetrac-gjorgenson mannequin opened this issue Jun 9, 2016 · 44 comments
Closed

Computing plane curve models for algebraic curves #20790

sagetrac-gjorgenson mannequin opened this issue Jun 9, 2016 · 44 comments

Comments

@sagetrac-gjorgenson
Copy link
Mannequin

sagetrac-gjorgenson mannequin commented Jun 9, 2016

Given a generic algebraic curve, compute a plane curve birational to it.

CC: @bhutz @miguelmarco

Component: algebraic geometry

Keywords: gsoc2016

Author: Grayson Jorgenson

Branch/Commit: 679338b

Reviewer: Miguel Marco, Ben Hutz

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

@sagetrac-gjorgenson sagetrac-gjorgenson mannequin added this to the sage-7.3 milestone Jun 9, 2016
@sagetrac-gjorgenson
Copy link
Mannequin Author

sagetrac-gjorgenson mannequin commented Jun 13, 2016

Branch: u/gjorgenson/ticket/20790

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 13, 2016

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

65a452c20790: merge with tickets 20697, 20676
309eb0320790: First attempt at implementing projections for projecitve curves.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 13, 2016

Commit: 309eb03

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2016

Changed commit from 309eb03 to 0de1323

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 15, 2016

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

0de132320790: implemented projections for affine curves.

@sagetrac-gjorgenson
Copy link
Mannequin Author

sagetrac-gjorgenson mannequin commented Jun 15, 2016

comment:4

I implemented a function to project affine curves that allows the user to specify coordinates to use in the projection. If the given coordinates yield a projection that doesn't have a curve as its image, an error will be raised. I also implemented a plane_projection function that doesn't take any user input but just tries to find two coordinates that will yield a projection that has a plane curve as its image. I think such a projection should always exist, but right now my approach to finding one is just trying all possible projections to two coordinates.

For projective curves, I added some functionality for working with curves defined over finite fields. Now the projection function will test all of the points of the ambient space of a curve over a finite field until it finds a point not on the curve. An error is returned if the curve contains all of the ambient space points.

@miguelmarco
Copy link
Contributor

comment:5

A few questions:

  • It could make sense in some cases to consider projections even if the image is not a curve (or if it is, but some components could be mapped to points). I think we should allow that, but maybe raising a warning about it.

  • In python (and sage), indices start in 0. Shouldn't we follow that approach when selecting the coordinates we project to? MAybe we could also allow a list (or tuple) of variables as input.

  • don't modify the input indices inside the function. That could have undesired side effects. Instead, create an internal copy.

  • I think you go into too much trouble to define the elimination ideal. Just calling elimination_idealon it should give you the ideal defining the image. Something like this:

sage: A.<x,y,z> = AffineSpace(QQ,3)
sage: C = Curve([y^7 - x^2 + x^3 - 2*z, z^2 - x^7 - y^2], A)
sage: I = C.defining_ideal()
sage: IE = I.elimination_ideal(z)
sage: IE
Ideal (y^14 + 2*x^3*y^7 - 2*x^2*y^7 - 4*x^7 + x^6 - 2*x^5 + x^4 - 4*y^2) of Multivariate Polynomial Ring in x, y, z over Rational Field
sage: A2 = AffineSpace(QQ, 2, (x,y))
sage: H = C.Hom(A2)
sage: phi = H((x,y))
sage: phi
Scheme morphism:
  From: Affine Curve over Rational Field defined by y^7 + x^3 - x^2 - 2*z, -x^7 - y^2 + z^2
  To:   Affine Space of dimension 2 over Rational Field
  Defn: Defined on coordinates by sending (x, y, z) to
        (x, y)
sage: C2 = Curve([A2.coordinate_ring()(i) for i in IE.gens()],A2)
sage: C2
Affine Plane Curve over Rational Field defined by y^14 + 2*x^3*y^7 - 2*x^2*y^7 - 4*x^7 + x^6 - 2*x^5 + x^4 - 4*y^2

should be enough to construct both the morphism and the image curve. Something similar could be done for the projective case.

  • Do we want to create new coordinates in the case of affine projection? or would it be better to keep the original ones? Maybe have an option for this?

  • I think it would me slightly more pythonic to return a tuple instead of a list.

  • In the projective case, ¿would it make sense to have an option to project to smaller subspaces?

  • In the plane projection method, instead of breaking the loop, just return L. Also, you could avoid the double loop by just iterating over the Subsets of the coordinates (maybe this is slower, but I think it would make the code easyer to read).

  • I am confused by this part of the code in the projective projection:

            l = list(PP.gens())
            for i in range(n+1):
                l[i] = 1
                while(F(l) == 0):
                    l[i] = l[i] + 1

I think that, in general, you could get simpler projections if you start with l[i] = 0 (for instance, most times you will get the projection from point [0:0:...: 0:1].

  • In this code, why do you create H in the first time? You redefine it just later without using it:
        H = Hom(PP, PP)
        # only need the first n coordinates of the change of coordinates map
        coords = [PP.gens()[i] - Q[i]/Q[n]*PP.gens()[n] for i in range(n)]
        # create the projection map onto the first n coordinates
        PP2 = ProjectiveSpace(self.base_ring(), n - 1)
        H = Hom(self, PP2)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 16, 2016

Changed commit from 0de1323 to d9d149f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 16, 2016

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

d9d149f20790: various changes from first review

@miguelmarco
Copy link
Contributor

comment:7

Looks good so far.

One addition that would be nice os to allow the user to choose the projection point. It may become useful at some point.

@miguelmarco
Copy link
Contributor

comment:8

By the way, instead of

 indices[len(indices) - 1]

you can just use

indices[-1]

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 18, 2016

Changed commit from d9d149f to 7e63954

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 18, 2016

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

7e6395420790: minor changes and option for projective projection to pass a point

@sagetrac-gjorgenson
Copy link
Mannequin Author

sagetrac-gjorgenson mannequin commented Jun 18, 2016

comment:10

Thanks, changes made!

@vbraun
Copy link
Member

vbraun commented Jun 21, 2016

comment:12

Reviewer name

@miguelmarco
Copy link
Contributor

Reviewer: Miguel Marco

@vbraun
Copy link
Member

vbraun commented Jun 23, 2016

comment:14

Merge conflict, merge in next beta...

@sagetrac-gjorgenson
Copy link
Mannequin Author

sagetrac-gjorgenson mannequin commented Jun 23, 2016

Changed commit from 7e63954 to 7e1f6cf

@sagetrac-gjorgenson
Copy link
Mannequin Author

sagetrac-gjorgenson mannequin commented Jun 23, 2016

New commits:

7e1f6cf20790: merge with 7.3 beta4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 15, 2016

Changed commit from 16d6207 to 8785032

@sagetrac-gjorgenson
Copy link
Mannequin Author

sagetrac-gjorgenson mannequin commented Aug 15, 2016

comment:23

Ok, the user can now specify an ambient space to project into in each of the projection functions. I also added some input tests.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 15, 2016

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

39108a120790: added some tests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 15, 2016

Changed commit from 8785032 to 39108a1

@sagetrac-gjorgenson
Copy link
Mannequin Author

sagetrac-gjorgenson mannequin commented Aug 15, 2016

comment:25

The projection functions now have tests to show passing an ambient space will ensure the projected curves are defined in that space.

@bhutz
Copy link

bhutz commented Aug 16, 2016

comment:26

looks good to me.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 20, 2016

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

67320bbMerge branch 'master' into u/gjorgenson/ticket/20790

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 20, 2016

Changed commit from 39108a1 to 67320bb

@sagetrac-gjorgenson
Copy link
Mannequin Author

sagetrac-gjorgenson mannequin commented Aug 20, 2016

comment:28

Fixed a merge conflict with the imports in affine_curve.py, projective_curve.py.

@vbraun
Copy link
Member

vbraun commented Aug 24, 2016

comment:30

Merge conflict, probably #21085

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 24, 2016

Changed commit from 67320bb to 679338b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 24, 2016

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

3dd26e721085: resolution of singularities implementation, first attempt
4a6bfbf21085: some minor changes
78e1ce221085: changes from review
1cbb35521085: generalized blowup() to work for any point on a given curve
d1b1edbMerge branch 'master' into u/gjorgenson/ticket/21085
78fdd8121085: add import back in that was accidentally removed during merge
50d05de21285: error in change_ring for affine morphisms
c6b7a4221085: merge with ticket 21285 for bug fix
bebf7b021085: minor improvements
679338b20790: merge with 21085 to fix conflicts

@sagetrac-gjorgenson
Copy link
Mannequin Author

sagetrac-gjorgenson mannequin commented Aug 24, 2016

comment:32

Ok, fixed the conflict with 21085. I don't think there are any other curve tickets closed after sage 7.4 beta1, so this should be good to go.

@vbraun
Copy link
Member

vbraun commented Aug 29, 2016

Changed branch from u/gjorgenson/ticket/20790 to 679338b

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

4 participants