#20790 closed enhancement (fixed)

Computing plane curve models for algebraic curves

Reported by: gjorgenson Owned by:
Priority: minor Milestone: sage-7.4
Component: algebraic geometry Keywords: gsoc2016
Cc: bhutz, mmarco Merged in:
Authors: Grayson Jorgenson Reviewers: Miguel Marco, Ben Hutz
Report Upstream: N/A Work issues:
Branch: 679338b (Commits) Commit: 679338b2f7fd3888f06fa4f5d1b69310339e1fd7
Dependencies: Stopgaps:

Description

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

Change History (34)

comment:1 Changed 18 months ago by gjorgenson

  • Branch set to u/gjorgenson/ticket/20790

comment:2 Changed 18 months ago by git

  • Commit set to 309eb0316c74e84cb8b7bd2fad0ab5514cf4d894

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.

comment:3 Changed 18 months ago by git

  • Commit changed from 309eb0316c74e84cb8b7bd2fad0ab5514cf4d894 to 0de1323a77147059de0f82c7a8642c44c31552ed

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

0de132320790: implemented projections for affine curves.

comment:4 Changed 18 months ago by gjorgenson

  • Status changed from new to needs_review

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.

comment:5 Changed 18 months ago by mmarco

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)

comment:6 Changed 17 months ago by git

  • Commit changed from 0de1323a77147059de0f82c7a8642c44c31552ed to d9d149f0b2aca63e2b4f97ee0d5d9e0b4763657b

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

d9d149f20790: various changes from first review

comment:7 Changed 17 months ago by mmarco

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.

comment:8 Changed 17 months ago by mmarco

By the way, instead of

 indices[len(indices) - 1]

you can just use

indices[-1]

comment:9 Changed 17 months ago by git

  • Commit changed from d9d149f0b2aca63e2b4f97ee0d5d9e0b4763657b to 7e639548c685b5d88c665bd582232abde8c79283

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

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

comment:10 Changed 17 months ago by gjorgenson

Thanks, changes made!

comment:11 Changed 17 months ago by mmarco

  • Status changed from needs_review to positive_review

comment:12 Changed 17 months ago by vbraun

Reviewer name

comment:13 Changed 17 months ago by mmarco

  • Reviewers set to Miguel Marco

comment:14 Changed 17 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict, merge in next beta...

comment:15 Changed 17 months ago by gjorgenson

  • Commit changed from 7e639548c685b5d88c665bd582232abde8c79283 to 7e1f6cf2732f6f832d5eb3d79d4785223e6f3335
  • Status changed from needs_work to needs_review

New commits:

7e1f6cf20790: merge with 7.3 beta4

comment:16 Changed 17 months ago by git

  • Commit changed from 7e1f6cf2732f6f832d5eb3d79d4785223e6f3335 to 98285148ebdb1c2c3cc189ec82716a80784c35f8

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

982851420790: merge with 7.3 beta5

comment:17 Changed 17 months ago by git

  • Commit changed from 98285148ebdb1c2c3cc189ec82716a80784c35f8 to ee363e4fc1c9be96b771f9fe1fd5f05ede4862ce

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

ee363e420790: merge with 7.3 beta6

comment:18 Changed 16 months ago by chapoton

  • Status changed from needs_review to needs_work

You now need to use relative import for constructor:

from .constructor import Curve

comment:19 Changed 16 months ago by git

  • Commit changed from ee363e4fc1c9be96b771f9fe1fd5f05ede4862ce to 16d6207d099a7368a20bb2c0527c120f5d51943c

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

c69bcd120790: removed constructor imports
16d620720790: merge with sage 7.3.rc0

comment:20 Changed 16 months ago by gjorgenson

  • Status changed from needs_work to needs_review

Ok, should be good now

comment:21 Changed 16 months ago by bhutz

  • Milestone changed from sage-7.3 to sage-7.4
  • Reviewers changed from Miguel Marco to Miguel Marco, Ben Hutz
  • Status changed from needs_review to needs_work

All tests currently pass, but in looking through the functionality I don't think the 'newvariables' approach is consistent with other parts of sage. the names of the variables does not specify the space. So even if you have two affine spaces of the same dimension, base_ring and variables, they will be isomorphic, but not the same object in Sage. I think it would be better to have the user give the space to project into.

Similarly for the 'same variables' boolean. They really aren't the same variables. I think this just leads to user confusions since now you have two 'x's that mean different things.

Finally, for plane projection, you don't allow this functionality.

I think in all cases the better solution is to allow the user to specify the space, not the names of the variables. Look, for example, at the affine_patch and projective_embedding functionality for subschemes.

Unrelated: the doc tests do not test the input checks nor do they test the 'newvariables' parameter.

comment:22 Changed 16 months ago by git

  • Commit changed from 16d6207d099a7368a20bb2c0527c120f5d51943c to 878503201d65e0ace5146466d706417ec383fc83

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

878503220790: changes from review

comment:23 Changed 16 months ago by gjorgenson

  • Status changed from needs_work to needs_review

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

comment:24 Changed 15 months ago by git

  • Commit changed from 878503201d65e0ace5146466d706417ec383fc83 to 39108a1cf5c2b84da83b01f7283390236852ae1d

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

39108a120790: added some tests

comment:25 Changed 15 months ago by gjorgenson

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

comment:26 Changed 15 months ago by bhutz

  • Status changed from needs_review to positive_review

looks good to me.

comment:27 Changed 15 months ago by git

  • Commit changed from 39108a1cf5c2b84da83b01f7283390236852ae1d to 67320bb67498749e9a85e7cb77f6bbd51a6fdad3
  • Status changed from positive_review to needs_review

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

comment:28 Changed 15 months ago by gjorgenson

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

comment:29 Changed 15 months ago by bhutz

  • Status changed from needs_review to positive_review

comment:30 Changed 15 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict, probably #21085

comment:31 Changed 15 months ago by git

  • Commit changed from 67320bb67498749e9a85e7cb77f6bbd51a6fdad3 to 679338b2f7fd3888f06fa4f5d1b69310339e1fd7

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

comment:32 Changed 15 months ago by gjorgenson

  • Status changed from needs_work to needs_review

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.

comment:33 Changed 15 months ago by bhutz

  • Status changed from needs_review to positive_review

comment:34 Changed 15 months ago by vbraun

  • Branch changed from u/gjorgenson/ticket/20790 to 679338b2f7fd3888f06fa4f5d1b69310339e1fd7
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.