Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#14353 closed enhancement (fixed)

Factor toric morphism into surjective and generically injective

Reported by: vbraun Owned by: AlexGhitza
Priority: major Milestone: sage-5.11
Component: algebraic geometry Keywords: toric, sd48
Cc: novoselt Merged in: sage-5.11.beta3
Authors: Volker Braun, Andrey Novoseltsev Reviewers: Andrey Novoseltsev, Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Attachments (6)

trac_14353_fan_cone_intersection.patch (2.7 KB) - added by vbraun 9 years ago.
Updated patch
trac_14353_factor_fan_morphism.patch (9.1 KB) - added by vbraun 9 years ago.
Updated patch
trac_14353_fan_morphism_factoring.patch (14.6 KB) - added by novoselt 9 years ago.
trac_14353_auxiliary_methods.patch (3.2 KB) - added by novoselt 9 years ago.
Made from Volker's patch
trac_14353_toric_morphism_factoring.patch (7.8 KB) - added by novoselt 9 years ago.
trac_14353_toric_morphism_composition.patch (4.0 KB) - added by vbraun 9 years ago.
Initial patch

Download all attachments as: .zip

Change History (20)

comment:1 Changed 9 years ago by vbraun

  • Cc novoselt added
  • Status changed from new to needs_review

comment:2 Changed 9 years ago by novoselt

  • Status changed from needs_review to needs_work

The support of a fan is in general not a cone at all! Unless I have a severe misunderstanding, it is a set-theoretic union of all its cones, e.g. for the fan generated by 1-cones (1,0) and (0,1) is not a cone. It would be great to have a method for support, but I am not sure how we can represent it at the moment apart from the fan itself.

Changed 9 years ago by vbraun

Updated patch

Changed 9 years ago by vbraun

Updated patch

comment:3 Changed 9 years ago by vbraun

  • Status changed from needs_work to needs_review

You are of course right. Fixed now.

comment:4 Changed 9 years ago by novoselt

Hey Volker, thanks for opening this ticket!

As I was thinking more about it, toric morphisms naturally factor into THREE: if f: Sigma --> Sigma', then we have

Sigma --f_s--> Sigma_s --f_b--> Sigma_i --f_i--> Sigma'

where f_s is surjection onto the fan which is the image of Sigma, f_i is injection of Sigma' intersected with the image sublattice, and f_b is a birational map between the intermediate toric varieties which does not have to be surjective or injective. The map from a single blowup chart into the original variety is precisely f_b with the other two being identities.

I would like to do the following:

  • adjust factoring to return three morphisms instead of two;
  • make intermediate fans live in the image_sublattice;
  • name them, perhaps, domain_fan_image (under the map of lattices) and codomain_fan_restriction (to the image_sublattice);
  • get rid of restrict_to_image method as it is misleading, if not wrong: restriction to the image of a toric morphism is not necessarily a toric morphism (it is not for the blow up chart). It does not seem like it is used anywhere but one place and basically it was trying to do the factoring in a not very good way.

I am working on it now on top of your patches, so let me know if you have severe objections!

comment:5 Changed 9 years ago by vbraun

I don't think there are particularly good names for the intermediate fans. Instead of trying to invent some name, we can just let a user use domain_fan() / image_fan() on the factored morphism.

We have a choice for Sigma_i. Just the intersection with the image sublattice will in general contain cones whose preimage under f_b is empty. We can leave (some of) those out and get a smaller Sigma_i. E.g. take the composition of the half blowup chart with the embedding C^2-> P^2. The natural thing to do is probably take this smaller fan. So f_i would be the embedding C^2->P^2 in the example.

I'm happy to get rid of restrict_to_image, I was tempted to do that myself....

Last edited 9 years ago by vbraun (previous) (diff)

comment:6 Changed 9 years ago by novoselt

Here is some work in progress, does not look good in the patch, but it removes restrict_to_image in the end and adds factor instead, diff blends these activities.

No other new methods apart is_birational which is not proper yet and there are some failures with composition of morphisms - I completely agree that these fans have to be accessed through morphisms, no need for new names.

As for Sigma_i indeed we have a choice, but I'd rather stick with "the big one" because:

  • it makes it independent of the domain fan, which is symmetrical to Sigma_s not depending on the codomain fan;
  • the corresponding variety will be a closed subvariety of the codomain;
  • it is easier to implement/faster to compute;
  • last but not least - I already have it in the patch ;-)

Changed 9 years ago by novoselt

Changed 9 years ago by novoselt

Made from Volker's patch

comment:7 Changed 9 years ago by novoselt

  • Authors changed from Volker Braun to Volker Braun, Andrey Novoseltsev
  • Description modified (diff)
  • Keywords toric added
  • Reviewers set to Andrey Novoseltsev, Volker Braun
  • Status changed from needs_review to needs_work
  • Work issues set to morphism composition

Hi Volker, I propose this version based on triple factorization for both fan and variety morphisms. I hope the documentation is written clearly and the example chosen gives a good idea what is what. The only remaining problem is that I really want to multiply factors back into the original one. For fan morphisms I just added __mul__ as they are standalone (although it may make sense to change that...), but toric morphisms should follow coercion rules, so implementing the double underscore method as it is done now is not acceptable. And even with it there is an issue of comparing, although it does seem that the result is correct.

Anyway: let me know what you think of composition and if the rest looks OK!

comment:8 Changed 9 years ago by vbraun

I think we shouldn't do more than implement the factorization in this ticket, fixing composition of morphisms is another task. Though right now this doctest fails, maybe you want to mark it as # known bug?

sage -t sage/schemes/toric/morphism.py
**********************************************************************
File "sage/schemes/toric/morphism.py", line 544, in sage.schemes.toric.morphism.SchemeMorphism_fan_toric_variety.factor
Failed example:
    prod(phi.factor()) == phi
Expected:
    True
Got:
    False

Changed 9 years ago by novoselt

comment:9 Changed 9 years ago by novoselt

  • Status changed from needs_work to needs_review
  • Work issues morphism composition deleted

Well, since composition was actually working and the problem was in comparison, I've added __cmp__ - composing factors to compare with the original is the most natural doctest here. All tests pass now, ready for review!

comment:10 Changed 9 years ago by novoselt

Apply trac_14353_fan_morphism_factoring.patch trac_14353_auxiliary_methods.patch trac_14353_toric_morphism_factoring.patch

Changed 9 years ago by vbraun

Initial patch

comment:11 Changed 9 years ago by vbraun

  • Description modified (diff)

comment:12 Changed 9 years ago by vbraun

  • Keywords sd48 added
  • Status changed from needs_review to positive_review

Positive review after in-persion discussion with Andrey

comment:13 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.11.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:14 Changed 9 years ago by jdemeyer

Follow-up: #14975. Please fix that soon or I will unmerge this ticket.

Note: See TracTickets for help on using tickets.