Opened 2 years ago

Closed 2 years ago

#20774 closed enhancement (fixed)

Basic singularity analysis for algebraic curves

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

Description (last modified by gjorgenson)

Compute the set of singular points of a generic algebraic curve and determine their multiplicities.

For plane curves, implement functionality to compute the tangents of the curve at a point, and to determine whether a singular point is ordinary or not.

Change History (18)

comment:1 Changed 2 years ago by gjorgenson

  • Branch set to u/gjorgenson/ticket/20774

comment:2 Changed 2 years ago by git

  • Commit set to 4655c72f7de4b9b251d0837cabf1f81a41454182

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

39dd21520676: Attempt to keep track of ambient spaces.
02349fe20676: projective_embedding revision and projective_closure.
ef65aca20676: Merge ticket 20697 into this ticket.
3ffc3aa20676: Merge ticket 20698 into this ticket.
c6fe84020676: implemented curve-level functions.
54f4e6220676: Merge again with 20697.
0039d7b20676: Update example strings to match changes from 20697.
9262ae520676: changes from review, and spacing fixes.
4fce7ed20774: Merge this ticket with 20676.
4655c7220774: Implement multiplicity, tangents, and is_ordinary_singularity

comment:3 Changed 2 years ago by git

  • Commit changed from 4655c72f7de4b9b251d0837cabf1f81a41454182 to e1f15291034546dd48d91284790df481af5aacdf

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

e1f152920774: Merge with 7.3 beta3

comment:4 Changed 2 years ago by gjorgenson

  • Description modified (diff)
  • Status changed from new to needs_review

comment:5 Changed 2 years ago by bhutz

  • Reviewers set to Ben Hutz
  • Status changed from needs_review to needs_work

some comments in no particular order

  • the values I've checked agree with Magma.
  • I would rather see a while loop than a for/break since you are using the loop variable later when you are getting an appropriate affine patch
  • examples are all QQ or number field. I'd like to see some other fields (assuming they work)
  • in multiplicity: why don't you change ring first to get the ordering, and then use R.ideal to create the ideal. That way you only have to change_ring once instead of twice.
  • OUTPUT: an integer. You don't have to make a list when it is a simple output
  • multiplicity = 0: do you really want to allow this? I think returning an error that the point is not on the curve may be more intuitive. If you change it: in tangents: if r < 1 isn't needed. Either way, this should be described in the doc
  • in is_ordinary: OUTPUT: Boolean. You should document that an error is returned for a non-singular point
  • in singular_points: I think putting F=F is better than adding the 0 as the first parameter.
  • do you want to add C.singular_subscheme()?
  • I see many places you are restricting to fields. Should there be a field subclass?
  • Things like Q.is_ordinary_singularity(), Q.tangents(), Q.multiplicity() don't work when Q is the curve point.
  • Q.is_singular() and C.is_singular(Q) would be nice to have.

comment:6 Changed 2 years ago by git

  • Commit changed from e1f15291034546dd48d91284790df481af5aacdf to e6f1a9364395b0507ba872b4addf5b11d3e17ef8

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

e6f1a9320774: Changes from first review.

comment:7 Changed 2 years ago by gjorgenson

Thanks. I added some examples with different fields. It looks like multiplicity works with RR and CC, and with finite fields. I think only number fields work with the tangents functions though, since they use the function factor().

I implemented an is_singular function which just uses the is_smooth functions of affine/projective subschemes. If given a point it returns whether it's singular or not, otherwise it returns whether the curve is singular or not. I added a singular_subscheme function as well.

Currently, only plane curves over general rings can be defined. In the space curve initialization, dimension is checked. Could we get around this by checking the dimension of the new curve created by embedding the defining polynomials of the original curve into the polynomial ring with coefficients from the field of fractions of the base ring (assuming it's an integral domain)?

If we add classes for curves over rings, would the naming scheme AffineCurve_ring, ProjectiveCurve_ring for generic curves over rings, and renaming AffineCurve? --> AffineCurve_field ProjectiveCurve? --> ProjectiveCurve_field be okay?

I haven't yet implemented the corresponding singularity analysis functions for points on curves; do you think it might be good for me to create classes for points on general curves first? Maybe similar to what's done in for points on elliptic curves?

comment:8 Changed 2 years ago by bhutz

At a quick glance the code changes look good, but due to traveling I won't get to really test them for a couple days.

I forgot we need to check dimension in the curve initialization. Yes, you could probably pass to the fractionfield in most cases and go from there (certainly for projective curves), but with that much complication, creating a field/ring class should be a different ticket. I consider it low priority and can be done possibly at a later date if you decide you have enough differentiated functionality to warrant it.

It does look to me like it is time for a point class for curves. Some things can be done in the corresponding point class for subschemes, but I think you have enough curve.point specific functionality to warrant its own class. This can be a different ticket where you create the class and add the corresponding singular analysis point functions. Most of the basic functionality for points can just inherit directly from appropriate subscheme point class.

comment:9 Changed 2 years ago by gjorgenson

  • Status changed from needs_work to needs_review

Okay, if classes and singularity analysis for points are implemented in another ticket, I think this one is ready for review. I opened #20811 for the points functionality. Is it okay to put the new point classes in the files for the subscheme point classes, or would it be better to make a points file in the curves folder for them?

comment:10 Changed 2 years ago by bhutz

  • Status changed from needs_review to needs_work

except for the merge conflict, this functionality looks fine.

comment:11 Changed 2 years ago by mmarco

Maybe you should add doctests that illustrate the possible errors raised.

comment:12 Changed 2 years ago by git

  • Commit changed from e6f1a9364395b0507ba872b4addf5b11d3e17ef8 to bfcd05e6885d0f66ee55556deaf2dacc792a8c96

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

1f0d7ab20774: Merge with 7.3 beta4.
bfcd05e20774: Added some error tests, and updated error messages.

comment:13 Changed 2 years ago by gjorgenson

Merging with beta4 broke the example in the singular_points function at line 260 of; too few points seem to be returned now.

It looks like this is due to the point search changes for 0 dimensional subschemes from #20780. Replacing the new point search with the old one from before the merge resolves the problem. Here's the failed example modified a bit:

R.<a> = QQ[]
K.<b> = NumberField(a^8 - a^4 + 1)
P.<x,y,z> = ProjectiveSpace(K, 2)
C = Curve([359/12*x*y^2*z^2 + 2*y*z^4 + 187/12*y^3*z^2 + x*z^4\
+ 67/3*x^2*y*z^2 + 117/4*y^5 + 9*x^5 + 6*x^3*z^2 + 393/4*x*y^4\
+ 145*x^2*y^3 + 115*x^3*y^2 + 49*x^4*y], P)
X = C.singular_subscheme()
Q = P([1/2*b^5 + 1/2*b^3 - 1/2*b - 1,1,0])
(1/2*b^5 + 1/2*b^3 - 1/2*b - 1 : 1 : 0)
X.rational_points() # Q is on X, but not in set of rational points anymore
[(2/3*b^4 - 1/3 : 0 : 1), (b^6 : -b^6 : 1), (-b^6 : b^6 : 1), (-2/3*b^4 + 1/3 : 0 : 1)]

I thought the point search method in #20780 looked good when I reviewed it, and I think this issue might just be from a weird special case. For this example, during the part of the algorithm using the affine patch corresponding to y = 1, the Groebner basis used is [x^2 + 2*x + 1/2*z^2 + 3/2, x*z + z, z^3 + z]. The rightmost polynomial has z = 0 among its roots, but substituting z = 0 and y = 1 into the next polynomial x*z + z yields 0, and I think this causes the algorithm to skip any points with z = 0, y = 1. But if those values had been kept for the last polynomial x^2 + 2*x + 1/2*z^2 + 3/2, substituting them in would give x^2 + 2*x + 3/2 which would give the x values of the missing points.

I think a way to resolve this might be to just add something like

    good = 1

after the if statement if L != 0: in line 159 of Doing this doesn't seem to break any examples, but does it mess up the algorithm at all?

comment:14 Changed 2 years ago by bhutz

Yes, I agree with that fix for points. It looks like it was dropping the point if the 'next' poly in the basis was already 0. Your 'else' makes sure it keeps that point.

comment:15 Changed 2 years ago by git

  • Commit changed from bfcd05e6885d0f66ee55556deaf2dacc792a8c96 to 4282a4ff695ae038c1b3268255e23f2f580af6c5

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

4282a4f20774: addressed small bug with point search

comment:16 Changed 2 years ago by gjorgenson

  • Status changed from needs_work to needs_review

comment:17 Changed 2 years ago by bhutz

  • Status changed from needs_review to positive_review

Interestingly the point issue came up in another ticket I was testing today as well. This fix also resolves those instances as well.

tests pass and docs build for me.

comment:18 Changed 2 years ago by vbraun

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