Opened 16 months ago

Closed 15 months ago

Last modified 15 months ago

#21085 closed enhancement (fixed)

Blow ups and resolution of singularities for 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: Ben Hutz
Report Upstream: N/A Work issues:
Branch: bebf7b0 (Commits) Commit:
Dependencies: #21168, #21285 Stopgaps:

Description (last modified by gjorgenson)

Implement functions to compute the blow up of an affine curve at a point, and to construct a nonsingular model for the curve by blowing up its singular points. These functions should describe the blow up/nonsingular model in terms of affine patches.

Change History (19)

comment:1 Changed 16 months ago by gjorgenson

  • Branch set to u/gjorgenson/ticket/21085

comment:2 Changed 16 months ago by git

  • Commit set to f5fc535f7ca0a493f7e8b0b33079b8ffbc466fc1

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

f5fc53521085: blow ups for affine curves

comment:3 Changed 16 months ago by git

  • Commit changed from f5fc535f7ca0a493f7e8b0b33079b8ffbc466fc1 to 3dd26e7cd33ac155e9d102952701b26c2d6f5b99

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

b923e3621085: update the affine blow up function.
112b608Merge branch 'master' into u/gjorgenson/ticket/21085
e4ea51e21168: change_ring implementation
b80b53321085: merge with ticket 21168 for access to change_ring function
3dd26e721085: resolution of singularities implementation, first attempt

comment:4 Changed 16 months ago by gjorgenson

  • Dependencies set to #21168
  • Description modified (diff)
  • Milestone changed from sage-7.3 to sage-7.4
  • Status changed from new to needs_review
  • Summary changed from Blow ups for affine/projective curves to Blow ups and resolution of singularities for curves

comment:5 Changed 16 months ago by git

  • Commit changed from 3dd26e7cd33ac155e9d102952701b26c2d6f5b99 to 4a6bfbfe819f427ec1485b8b1e8ea0a704af6682

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

4a6bfbf21085: some minor changes

comment:6 Changed 16 months ago by bhutz

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

I think this is looking good overall. I have a few nits and a couple suggestions.

  • you have no examples over A^3 or higher even though it is in the general curve class
  • I'm not sure I like the behavior that you get when you resolve a curve where all the singularities are not rational. You essentially get the curve itself back. Perhaps you should raise an error of the form "no rational singularities, use extend=True" or something like that.
  • you should explicitly use the keyword in examples: 'extend=True' instead of just 'True'
  • For the extend = True example, here is one that takes < 1s instead of the 3s that yours takes. I didn't look closely at yours, so if it tests additional functionality, then by all means leave it.
    C=A.curve(x^4 + 2*x^2 + y^3 + 1)
    
  • I was testing blowup() and really wanted to pass it a point other than the original. Is that a pain, to translate the point and then translate back?
  • the error cases are not run in the examples and they should be
  • in extension(): two code improvements
L = [pt[j] for j in range(len(self.ambient_space().gens())) for pt in pts]}}

can be the simpler

L = [t for pt in pts for t in pt ]

and

K = number_field_elements_from_algebraics(L)[0]
return [K, self.base_ring().embeddings(K)[0]]

can be

K,a,phi = ...
return [K,phi]

since number_field_elements_from_algebraic returns the embedding it used, so you can return that instead of just the first element of embeddings.

comment:7 Changed 16 months ago by git

  • Commit changed from 4a6bfbfe819f427ec1485b8b1e8ea0a704af6682 to 78e1ce23310ece13ece4eec995b849e5fc95ccf4

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

78e1ce221085: changes from review

comment:8 Changed 16 months ago by gjorgenson

  • Status changed from needs_work to needs_review

Thanks! Made most of the changes.

There were two A^3 examples in blowup() and resolution_of_singularities() so I left those as is, but I could still add some more higher dim. examples (the output gets long quickly).

For the extension() function I tried using the composite_fields functionality that number fields have; I think this might be cleaner than the approach I was trying with passing the generator of a number field as a QQbar element to number_field_elements_from_algebraics along with the other QQbar numbers in order to get a bigger field.

I think I could get blowup() to work for arbitrary points. After moving a point to the origin and blowing up, I'm not sure how to transform back though as the blowup will live in a mixed product space. I think maybe just constructing the ideal for the blow up centered at a different point directly might work.

Given a point P = (p_1,...,p_n), I think the ideal of the blow up at that point would be generated by the polynomials (x_i - p_i)s_j - (x_j - p_j)s_i where the x_i are the coords for the affine component, and the s_i are the coords of the projective component of the product space. I tried something like this in my first attempt at the blowup function, but was unsure whether it was the right construction. Do you think this gives the right ideal? It seems like it should still give the closure of the graph of the rational map defined (x_1,...,x_n) ---> (x_1 - p_1 :... : x_n - p_n) which I think is how the blow up at P is defined.

comment:9 Changed 16 months ago by bhutz

THis new resolution functionality looks fine to me.

For the blow-up, yes that should be the correct ideal to blow up not at the origin. That is almost certainly easier than translating.

comment:10 Changed 15 months ago by git

  • Commit changed from 78e1ce23310ece13ece4eec995b849e5fc95ccf4 to 1cbb355d5f51adf31d06de992798825cead6566b

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

1cbb35521085: generalized blowup() to work for any point on a given curve

comment:11 Changed 15 months ago by git

  • Commit changed from 1cbb355d5f51adf31d06de992798825cead6566b to 78fdd8169cf389b44ff87dcfd0712bc66338114b

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

d1b1edbMerge branch 'master' into u/gjorgenson/ticket/21085
78fdd8121085: add import back in that was accidentally removed during merge

comment:12 Changed 15 months ago by gjorgenson

Ok, I generalized the blowup() function to work for arbitrary points. This simplified some of the code in resolution_of_singularities since there's no longer a need to keep track of change of coordinate maps used to move points to the origin. However, I ran into a couple of issues when testing some higher dimension examples.

It seems that the way I was trying to isolate the strict transforms of the curves wrt the blow ups isn't always sufficient for higher dimension curves, and sometimes the resulting curves would be reducible even though the original curves were irreducible. To address this, the blowup() function now looks at the irreducible components of the transformed curve (after substituting the relations that hold in a particular chart of the blowup) and ignores any component that lies within the exceptional divisor.

In implementing the above I noticed another issue which occurs when the transform of the curve coincides with the exceptional divisor. An example of this is when blowing up the line x at the origin; in one of the charts the transformed line and the exceptional divisor both seem to be defined by x. If I understand correctly the strict transform is the closure of the preimage by the projection map of the line minus the origin, which I think should be empty in that chart. The output in the current implementation is:

A.<x,y> = AffineSpace(QQ, 2)
C = A.curve([x])
C.blowup([0,0])[0]
(Closed subscheme of Affine Space of dimension 2 over Rational Field
defined by: 1, Affine Plane Curve over Rational Field defined by s0)

Should the strict transform be empty in the first chart, and if so, would it be better to leave that chart out of the output?

I also encountered an unrelated issue with applying change_ring to the transition maps when extending the base field. A simplified example of the problem is:

K.<a> = QuadraticField(-1)
A.<x,y> = AffineSpace(K, 2)
H = End(A)
phi = H([x/y, y])
emb = K.embeddings(QQbar)[0]
phi.change_ring(emb)

which raises a TypeError without any description. This appears to only be an issue when there are actual fraction field elements defining the morphism. The line that breaks is R = self.coordinate_ring().hom(R, T.ambient_space().coordinate_ring()) in the change_ring function for the SchemeMorphism_polynomial class of generic/morphism.py. Is this actually a bug, or perhaps am I just misusing the functionality?

comment:13 Changed 15 months ago by bhutz

Yes, that is a bug. In that case the coordinate ring of the map was a fraction field, so it failed to create the hom. It is now fixed in #21285, which needs review.

comment:14 Changed 15 months ago by bhutz

  • Status changed from needs_review to needs_work

This is getting a little beyond my expertise, but what are describing sounds correct to me. As for what to do with the empty chart. I would prefer to return it so that there are 'right' number of charts. It is clearly empty based on the defining equations.

For the blow-up at points. A couple things. While it is fine to accept a list as input and do the coercision in the function, I think it would be better to pass in actual points in the examples, maybe a curve point for one and an affine space point for another. Also, I'd like to see it default to the origin, so that the point parameter is optional.

comment:15 Changed 15 months ago by git

  • Commit changed from 78fdd8169cf389b44ff87dcfd0712bc66338114b to bebf7b02f5cbe21fe85975821414b78fa65efadc

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

50d05de21285: error in change_ring for affine morphisms
c6b7a4221085: merge with ticket 21285 for bug fix
bebf7b021085: minor improvements

comment:16 Changed 15 months ago by gjorgenson

  • Dependencies changed from #21168 to #21168, #21285
  • Status changed from needs_work to needs_review

Alright, I made the changes and did some more functionality testing. So far everything seems to be working as intended on the examples I've come up with, and I think this should be good for review again.

I've also been trying to implement a projective blow up function; for a projective curve defined by f_1,...,f_m in P^n, I was thinking to use the ideal generated by f_1,...,f_m, x_i*y_j - x_j*y_i for i,j = 0,...,n-1 in the product space P^n\times P^{n-1} to define the blow up of the curve at the point (0:...:0:1), where the x_i are the coords for P^n and the y_i are the coords for P^{n-1}. However I'm not sure this is actually the correct ideal, and think it might be better to leave implementing projective blow ups at points for when more general functionality for blowing up along subschemes is implemented.

comment:17 Changed 15 months ago by bhutz

  • Status changed from needs_review to positive_review

This seems fine to me, so I'll mark it positive.

As for the projective blow-up. Those equations don't really make a lot of sense. I was trying to think of a general form for a projective blow-up, but really, this is a local object and the most natural way to think of it is through affine charts anyway.

So I'm fine with holding off on projective blow-ups.

comment:18 Changed 15 months ago by vbraun

  • Branch changed from u/gjorgenson/ticket/21085 to bebf7b02f5cbe21fe85975821414b78fa65efadc
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:19 Changed 15 months ago by jpflori

  • Commit bebf7b02f5cbe21fe85975821414b78fa65efadc deleted

Any chance some of you have a look at https://trac.sagemath.org/ticket/17254#comment:364 ? With the Singular upgrade a few signs changed in the doctests from here and we have no idea if the result is still valid (although it does not look obviously false).

Note: See TracTickets for help on using tickets.