Opened 8 months ago
Closed 6 months ago
#20895 closed enhancement (fixed)
Computing ordinary models of plane curves
Reported by:  gjorgenson  Owned by:  

Priority:  minor  Milestone:  sage7.4 
Component:  algebraic geometry  Keywords:  gsoc2016 
Cc:  bhutz, mmarco  Merged in:  
Authors:  Grayson Jorgenson  Reviewers:  Ben Hutz 
Report Upstream:  N/A  Work issues:  
Branch:  6ef31f5 (Commits)  Commit:  6ef31f5e81344759cc6a27941cd30ace24f6de9f 
Dependencies:  Stopgaps: 
Description
Given a plane curve, it is possible to transform it into a plane curve with only ordinary singularities via application of a finite sequence of quadratic transformation maps.
Implement a function at the curve class level to apply the standard quadratic transformation (the birational automorphism of P2
sending (x : y : z)
to (yz : xz : xy)
) and a function to transform a given plane curve into one with only ordinary singularities.
Change History (18)
comment:1 Changed 8 months ago by
 Branch set to u/gjorgenson/ticket/20895
comment:2 Changed 8 months ago by
 Commit set to 54a991e606b87e665bef1ea53c229f5a5df265f8
comment:3 Changed 8 months ago by
Okay, here's my first attempt at the implementation. I improved the affine tangents function to work with QQbar, and added some helper functions to apply the standard Cremona transformation, find the equation of the line between two projective plane points, and to move a curve into excellent position as defined in Fulton's alg. curves book. I haven't yet been able to find a good way to mitigate the dependency on QQbar, so right now I just restrict to QQbar curves entirely. Moving curves to excellent position seems to take > 2 seconds even for basic curves and quickly becomes impractical for more complicated curves. The new functionality does seem to be working correctly on the basic examples I've tested though.
Do you think it's possible to reduce the dependency on QQbar computations?
comment:4 Changed 8 months ago by
Take a look at the .as_number_field_element()
method of algebraic numbers, and the number_field_elements_from_algebraics
function. They allow you to get a number field to embed your numbers. So you can proceed this way: use QQbar just to compute the roots you need. Once done that, find a number field where everything you need may fit (that is, it should be an extension of your current field that contains the needed elements), and extend your base ring. From there, you can keep working in a concrete number field which should be much faster than QQbar.
comment:5 Changed 8 months ago by
 Commit changed from 54a991e606b87e665bef1ea53c229f5a5df265f8 to 1c91bf1b7fa839e488e23306acf6dbf78720b77f
Branch pushed to git repo; I updated commit sha1. New commits:
1c91bf1  20895: some revisions

comment:6 Changed 8 months ago by
 Commit changed from 1c91bf1b7fa839e488e23306acf6dbf78720b77f to 132dfb58a02856fb2650d17330866def4ddbb175
Branch pushed to git repo; I updated commit sha1. New commits:
132dfb5  20895: more efficient implementation

comment:7 Changed 8 months ago by
 Commit changed from 132dfb58a02856fb2650d17330866def4ddbb175 to dd402019c59ce2889d22933d3167079f0faf5997
Branch pushed to git repo; I updated commit sha1. New commits:
dd40201  20895: ordinary_model revision, and some minor fixes

comment:8 Changed 8 months ago by
Alright, I experimented with embedding into numberfields, but after doing some timing analysis, I found that the biggest use of time was finding intersection points of a given curve with the lines used to create a change of coordinates map to move the curve into excellent position. I tried revising the excellent_position and ordinary_model functions in the first of the last three commits to reduce the costs of these computations, but they still used a lot of time even for simple examples.
In the last two commits I tried a different approach and gave the excellent_position function an option to accept a list/tuple of three points to use to create the transformation (without checks), and modified the ordinary model function so that it now creates lists of vertices incrementally without explicitly checking that they put the curve into excellent position and passes them to excellent_position. To verify that the nonordinary singularities do become resolved, I'm using that after every application of excellent_position + quadratic_transformation, if the given curve was actually put into excellent position, the resulting curve should either have a smaller apparent genus (arithmetic genus  sum m*(m1)/2 as m runs over the curve's singular point multiplicities), or should have fewer nonordinary singularities. This gives an upper bound for the number of applications of excellent_position + quadratic_transformation needed to resolve the nonordinary singularities, and so if the nonordinary singularities are not resolved after this number of transformations, a new set of vertices is used.
So far this seems to work pretty quickly for curves of degree < 5, but is somewhat hitormiss for higher degree curves. The transformed curves can also have high degrees when multiple transformations are needed to resolve all of the nonordinary singularities, and sometimes don't seem very practically useful. I think the code is a bit too much of a mess right now for this to be ready for review, but does the method of implementation seem okay so far? Do you think there's a way I can make the transformations nicer?
comment:9 Changed 7 months ago by
 Commit changed from dd402019c59ce2889d22933d3167079f0faf5997 to 837ddb8b01cb421e7568545f8f2181c29a47a389
comment:10 Changed 7 months ago by
 Status changed from new to needs_review
I implemented a new approach for excellent_position that no longer requires computing intersection points and works for number fields. I think the computations are faster now and I revised ordinary_model to leave the checks to excellent_position. ordinary_model works for number fields as well by returning a curve defined over an extension if any of the coordinates of the nonordinary singularities are not contained in the original base field.
I also cleaned up the code and removed unnecessary changes that were implemented in previous commits, such as the line function for projective space.
comment:11 Changed 6 months ago by
 Commit changed from 837ddb8b01cb421e7568545f8f2181c29a47a389 to 71bd9a091149b640499c1d168845035b39fd0dcc
comment:12 Changed 6 months ago by
 Milestone changed from sage7.3 to sage7.4
I made some improvements to the functionality here and I think this should be ready for review.
Changes made in this ticket up to this point have been to add an ordinary_model function along with two helper functions, quadratic_transform and excellent_position, to the projective plane curve class. The is_ordinary_singularity and tangents functions for affine curves were also modified to reduce the need for QQbar computations.
Overall the ordinary_model function appears to be working properly, but still becomes very slow for most curves of degree > 5. I think this slowdown is mainly due to the rate at which the degrees of the quadratic transforms of the curves can grow.
comment:13 Changed 6 months ago by
 Reviewers set to Ben Hutz
 Status changed from needs_review to needs_work
I did not find any functionality issues, but here are some comments
 in affine_curve.py is there a reason you have
from sage.arith.misc import binomial
in the function and not with the rest of the imports
 in affine tangents()
t = T.degree(vars[0]) for monom in T.monomials(): if monom.degree(vars[0]) < t: t = monom.degree(vars[0]
seems to just be doing
min([e1 for e1,e2 in T.exponents()])
is that right? If so, you can simplify both instances.
 in is_ord : projective
C = self.affine_patch(i) Q = list(P) t = Q.pop(i) Q = [1/t*Q[j] for j in range(self.ambient_space().dimension_relative())]
You can just say
C(Q.dehomogenize(i))
to get the affine point
 in excellent position: projective
d = self.defining_polynomial().degree()
is there no C.degree() function for plane curves??
if all([g.degree(PP.gens()[0]) > 0 for g in T.monomials()]):
seems simpler as
if all([e[0] > 0 for e in T.exponents()]):
also this function fails over QQbar since .divides() does not work. You should either fix that or specify in the documentation that you need a number field
 in def ordinary_model(self):
Return an ordinary plane curve model of this curve.
Actually you are returning the morphism from the curve to an ordinary model. You should either match the description of excellent position or return the curve and add a return_mapping parameter. Either way, I think both functions should do the same thing.
I'd also like an example to show that all singularities are now ordinary.
[C.is_ordinary_singularity(Q) for Q in C.singular_points()]
in extension()
not sure why you are doing this:
# make sure the defining polynomial variable names are the same for K, N N = NumberField(K.defining_polynomial().parent()(F.defining_polynomial()), str(K.gen()))
because
sage: K.<v>=QuadraticField(2) sage: N = NumberField(K.defining_polynomial().parent()(x^3+2), str(K.gen())) sage: N.gen()==K.gen() False
comment:14 Changed 6 months ago by
 Commit changed from 71bd9a091149b640499c1d168845035b39fd0dcc to 6ef31f5e81344759cc6a27941cd30ace24f6de9f
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
9828514  20790: merge with 7.3 beta5

ee363e4  20790: merge with 7.3 beta6

c69bcd1  20790: removed constructor imports

16d6207  20790: merge with sage 7.3.rc0

8785032  20790: changes from review

39108a1  20790: added some tests

67320bb  Merge branch 'master' into u/gjorgenson/ticket/20790

679338b  20790: merge with 21085 to fix conflicts

ea2970f  20895: merge with ticket 20790 to fix conflicts

6ef31f5  20895: fix example

comment:15 Changed 6 months ago by
 Status changed from needs_work to needs_review
Thanks for reviewing this. I was a bit busy with preparing for the semester and wasn't able to finish working on the next update until now.
I made the suggested changes and also merged this with ticket 20790 (has been closed) to fix a conflict with the imports. The projective subscheme multiplicity and intersection_multiplicity functions now use the point dehomogenize function as well for cleaner code. I also added a degree function override for projective plane curves which just returns the degree of the defining polynomial of the curve to avoid using the slower Hilbert polynomial computation.
In
N = NumberField(K.defining_polynomial().parent()(F.defining_polynomial()), str(K.gen()))
I don't think str(K.gen())
is needed and can be replaced with an arbitrary variable name, but the reason I am using K.defining_polynomial().parent()(F.defining_polynomial())
is to make the defining polynomials of N
and K
have the same variable name. If the names are different the composite_fields function raises an error:
R.<x> = QQ[] S.<y> = QQ[] N.<a> = NumberField(x^2 + 1) M.<b> = NumberField(y^2  7) N.composite_fields(M) Traceback (click to the left of this block for traceback) ... sage.libs.pari.handle_error.PariError: inconsistent variables in polcompositum, x != y
comment:16 Changed 6 months ago by
 Status changed from needs_review to positive_review
The updates look fine to me and all tests still pass.
comment:17 Changed 6 months ago by
OUPUT ?
comment:18 Changed 6 months ago by
 Branch changed from u/gjorgenson/ticket/20895 to 6ef31f5e81344759cc6a27941cd30ace24f6de9f
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
20895: some pieces of functionality
20895: merge with ticket 20839.
20895: first pass at implementation