Opened 21 months ago
Closed 20 months ago
#20774 closed enhancement (fixed)
Basic singularity analysis for algebraic curves
Reported by:  gjorgenson  Owned by:  

Priority:  minor  Milestone:  sage7.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 )
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 21 months ago by
 Branch set to u/gjorgenson/ticket/20774
comment:2 Changed 21 months ago by
 Commit set to 4655c72f7de4b9b251d0837cabf1f81a41454182
comment:3 Changed 21 months ago by
 Commit changed from 4655c72f7de4b9b251d0837cabf1f81a41454182 to e1f15291034546dd48d91284790df481af5aacdf
Branch pushed to git repo; I updated commit sha1. New commits:
e1f1529  20774: Merge with 7.3 beta3

comment:4 Changed 21 months ago by
 Description modified (diff)
 Status changed from new to needs_review
comment:5 Changed 21 months ago by
 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 nonsingular 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 21 months ago by
 Commit changed from e1f15291034546dd48d91284790df481af5aacdf to e6f1a9364395b0507ba872b4addf5b11d3e17ef8
Branch pushed to git repo; I updated commit sha1. New commits:
e6f1a93  20774: Changes from first review.

comment:7 Changed 21 months ago by
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 ell_point.py for points on elliptic curves?
comment:8 Changed 21 months ago by
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 21 months ago by
 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 20 months ago by
 Status changed from needs_review to needs_work
except for the merge conflict, this functionality looks fine.
comment:11 Changed 20 months ago by
Maybe you should add doctests that illustrate the possible errors raised.
comment:12 Changed 20 months ago by
 Commit changed from e6f1a9364395b0507ba872b4addf5b11d3e17ef8 to bfcd05e6885d0f66ee55556deaf2dacc792a8c96
comment:13 Changed 20 months ago by
Merging with beta4 broke the example in the singular_points function at line 260 of curve.py; 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]) X(Q) (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
else: new_points.append(P) good = 1
after the if statement if L != 0:
in line 159 of projective_homset.py. Doing this doesn't seem to break any examples, but does it mess up the algorithm at all?
comment:14 Changed 20 months ago by
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 20 months ago by
 Commit changed from bfcd05e6885d0f66ee55556deaf2dacc792a8c96 to 4282a4ff695ae038c1b3268255e23f2f580af6c5
Branch pushed to git repo; I updated commit sha1. New commits:
4282a4f  20774: addressed small bug with point search

comment:16 Changed 20 months ago by
 Status changed from needs_work to needs_review
comment:17 Changed 20 months ago by
 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 20 months ago by
 Branch changed from u/gjorgenson/ticket/20774 to 4282a4ff695ae038c1b3268255e23f2f580af6c5
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
20676: Attempt to keep track of ambient spaces.
20676: projective_embedding revision and projective_closure.
20676: Merge ticket 20697 into this ticket.
20676: Merge ticket 20698 into this ticket.
20676: implemented curvelevel functions.
20676: Merge again with 20697.
20676: Update example strings to match changes from 20697.
20676: changes from review, and spacing fixes.
20774: Merge this ticket with 20676.
20774: Implement multiplicity, tangents, and is_ordinary_singularity