Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toric fibration morphisms #12892

Closed
vbraun opened this issue Apr 30, 2012 · 78 comments
Closed

Toric fibration morphisms #12892

vbraun opened this issue Apr 30, 2012 · 78 comments

Comments

@vbraun
Copy link
Member

vbraun commented Apr 30, 2012

This ticket provides more morphisms that are associated to toric varieties:

  • embedding of an orbit closure
  • embedding of a fiber of a toric morphism
  • pull-back of divisors

Use the git branch!

Depends on #12361
Depends on #13023
Depends on #14353

CC: @novoselt @sagetrac-jkeitel

Component: algebraic geometry

Keywords: sd40.5 sd53 toric

Author: Volker Braun

Branch/Commit: b3f06ed

Reviewer: Andrey Novoseltsev

Issue created by migration from https://trac.sagemath.org/ticket/12892

@vbraun

This comment has been minimized.

@vbraun
Copy link
Member Author

vbraun commented Apr 30, 2012

Author: Volker Braun

@novoselt
Copy link
Member

novoselt commented May 2, 2012

comment:2

This looks very cool, I plan to go over details in a couple weeks or sooner.

@novoselt
Copy link
Member

novoselt commented May 2, 2012

Reviewer: Andrey Novoseltsev

@vbraun
Copy link
Member Author

vbraun commented May 2, 2012

comment:3

In 3 weeks we can also talk about it in person in Seattle :-)

@novoselt
Copy link
Member

Dependencies: #12361

@novoselt
Copy link
Member

comment:5

I find the first patch a bit difficult to understand due to mixing several things defining the embedding: a cone and two dictionaries of rays. Also defining_cone argument is not documented in the constructor of the orbit morphism.

Orbits are in 1:1 correspondence with cones of the original fan, so it makes perfect sense to pass this information and store it (as it is currently done in the patch).

Mathematically, this is all that is needed, but since we have so far issues with supporting quotient lattices and instead the fan of the orbit lives in a "regular lattice", we need to keep the correspondence somehow. As it is done by some matrix, perhaps that's what we need to pass to the constructor and store.

Instead, the current version constructs a codomain_ray->domain_ray dictionary, it is passed to the constructor and constructor reverses it into domain_ray->codomain_index dictionary. Note that the map does not have to be one-to-one for non-simplicial fans, so this dictionary just picks some random representative for a domain ray. The choice may affect the decision on whether embedding can be realized as a polynomial map or not.

I also think it is confusing to store non-primitive generators for rays and treat a ray as not found if a non-primitive generator is found. We do represent rays throughout the code just by their generators, but it is always assumed that they are normalized.

As a feature request it would be nice to have support for maps given by homogeneous polynomials in the other direction, i.e. a map from the 12 orbit of P112 to P1 can be given as (0:z1:z2) -> (z1^2:z2) and this will work for all orbits with powers corresponding to that "non-primitivity of generators". Or is it already implemented and I am missing something?

Anyway, concretely: how about passing and storing defining_cone and projection_matrix as base pieces of data and relying on them for consecutive computations?

@novoselt
Copy link
Member

comment:6

For the patchbot (which should read the description...)

Apply trac_12892_orbit_closure_morphism.patch, trac_12892_toric_morphism_fibers.patch, trac_12892_toric_morphism_divisors.patch

@vbraun
Copy link
Member Author

vbraun commented May 25, 2012

comment:7

As discussed with Andrey, the existence of polynomial map doesn't depend on the choices made.

Updated patch to add some clarifications.

@vbraun
Copy link
Member Author

vbraun commented May 25, 2012

Changed keywords from none to sd40.5

@novoselt
Copy link
Member

comment:9

OK to the first patch!

@novoselt
Copy link
Member

comment:10

For the second patch:

  1. relative_star_generators does not have INPUT/OUTPUT and in general it would be nice to have a clear description of what it does.
  2. Can we please rename fiber to generic_fiber? (I would expect that fiber would return a particular one based on some input.) Also - why the documentation says that it returns a connected component, isn't it unique for a generic fiber?
  3. I also got confused by fiber_component name thinking it computes the fiber over points corresponding to higher-dimensional cones of the codomain. After some more thinking and reading I think that it is indeed the correct name, but would be nice to describe in the documentation the structure of non-generic fibers and why it makes more sense to work with components corresponding to domain cones rather than fibers of codomain ones.
  4. fiber_component and fiber_dimension also lack INPUT/OUTPUT blocks.
  5. SchemeMorphism_fan_fiber_toric_variety input documentation does not match the code.
  6. Perhaps the name of the class can be changed to ..._fiber_component_... since it does not operate with the whole fiber.
  7. "Defined by embedding the fiber irreducible component defined by the primitive preimage cone 1-d cone of Rational polyhedral fan in 4-d lattice N." does not read. While I was trying to reformulate it, I became unsure of this class at all. Isn't it just about embedding a torus orbit closure into the original toric variety? I.e. the toric morphism and fibers are not important?

@novoselt
Copy link
Member

Work Issues: comments and rebasing

@novoselt
Copy link
Member

Changed dependencies from #12361 to #12361, #13023

@vbraun
Copy link
Member Author

vbraun commented May 27, 2012

Updated patch

@vbraun
Copy link
Member Author

vbraun commented May 28, 2012

comment:12

Attachment: trac_12892_orbit_closure_morphism.patch.gz

I've updated the paths for #13023, and checked that all doctests pass.

@novoselt

This comment has been minimized.

@novoselt
Copy link
Member

novoselt commented Jun 5, 2012

comment:13

Beginning of the reviewer patch: I changed fiber to fiber_generic and want to change fiber_component to something more accurate since it does not necessarily return an irreducible component of a fiber, but perhaps a smaller dimensional piece. How about fiber_orbit_closure? That's how the documentation describes the output anyway.

@novoselt
Copy link
Member

comment:14
sage: P1 = toric_varieties.P1()
sage: f = P1.hom(matrix([2]), P1)
sage: f.fiber_orbit_closure(P1.fan(1)[0])
Traceback (most recent call last):
...
ArithmeticError: Argument gens (= [(1/2)]) does not generate a submodule of self.

This should either give a meaningful error message or better yet just work. I'll try to take care of it.

@vbraun
Copy link
Member Author

vbraun commented Jun 27, 2012

comment:15

The problem is of course that z |-> z^2 has two points as the fiber over {+1}. This is called the index in Hu-Liu-Yau. So we can either

a) Ignore it and just return one irreducible component

b) Return a pair (toric variety, index)

c) Return the different connected components (fiber, fiber, ..., fiber). Note that the embedding map differs by some roots of unity so you may not be able to write it over QQ...

@novoselt
Copy link
Member

comment:16

Yeap, I am working on it - had to brush up the math definitions and then a non-trivial random example exposed some issues with current code which I am fixing. Regarding returned values, I didn't make up my mind yet, but just ignoring other components is a bad choice, I think - even if we return only one there should be another method that will return their number (and I almost have it working, I think).

@vbraun
Copy link
Member Author

vbraun commented Jun 28, 2012

comment:17

Thinking about it, returning a pair (fiber, index) or perhaps a dict seems like the best choice. Having to call another method to get the index isn't easy to discover.

@novoselt
Copy link
Member

comment:48

Made square map work. Also tried to use quotient lattices more but without much success.

What exactly is _image_ray_multiplicity mathematically? I am confused by picking the maximum one - why don't other matter?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 28, 2013

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

[changeset:6b4b318]Treat generic fiber correctly as a fiber component.
[changeset:317dd40]Fixes to deal with maps between sublattices and virtual rays.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 28, 2013

Changed commit from 943e71b to 6b4b318

@novoselt
Copy link
Member

comment:50

Thinking of making aris (a variant of spelling arris, "the sharp edge or salient angle formed by the meeting of two surfaces especially in moldings") an alias for ambient_ray_indices ;-) Would make doctest a bit shorter.

Still have a bug in fan isomorphism and another one in representing morphisms as polynomial maps. Problems tend to stem from either fans/morphisms involving sublattices, which seem to be easy to fix, or from not dealing properly with torus factors, which are more involved.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 28, 2013

Changed commit from 6b4b318 to e79e840

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 28, 2013

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

[changeset:e79e840]Added a long fibration example. One doctest failure due to fan isomorphism bug.

@novoselt
Copy link
Member

comment:52

Jan - I think that embedding construction code here is stabilized (unless Volker has any objections) and it is OK to merge your changes on top of this.

@novoselt
Copy link
Member

comment:53

Volker - How about adding .coordinates() or something like this method to PointCollection which will return a point collection with the same points as the original, but written in terms of the basis of the original module? I think this will help in dealing with sublattices and quotients, e.g. there is a bunch of methods for 2d fans that test dimension of the lattice, but need to take into account that actual points may have more than 2 components. This is the reason for the failing doctest where P2 is represented by a fan in a sublattice.

@novoselt
Copy link
Member

Changed work issues from comments to fan isomorphism bug

@novoselt
Copy link
Member

Changed keywords from sd40.5 to sd40.5 sd53

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.0, sage-6.1 Dec 17, 2013
@novoselt
Copy link
Member

comment:56

Hey Volker, since you have started merging branches, I propose marking the failing doctest for P2 as known bug and merging this one as well - correct fix for P2 will require some careful work and it is certainly not introduced by this ticket. Do you have any objections to my changes so far?

@vbraun
Copy link
Member Author

vbraun commented Dec 19, 2013

comment:57

I agree with your changes, thanks! I'm also fine with marking the P2 thing as known bug and split it of to a separate ticket. Please go ahead!

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 26, 2014

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

b3f06edMarked failing doctest as a known bug #16012.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 26, 2014

Changed commit from e79e840 to b3f06ed

@novoselt
Copy link
Member

Changed work issues from fan isomorphism bug to none

@novoselt
Copy link
Member

Changed keywords from sd40.5 sd53 to sd40.5 sd53 toric

@novoselt
Copy link
Member

comment:60

Hey Volker - sorry for spending 3 months on these 3 doc lines, but it is still seems to be mergeable and tests run fine for me. I'll take the liberty to switch to positive review since this change was preapproved.

@vbraun
Copy link
Member Author

vbraun commented Apr 1, 2014

Changed branch from u/novoselt/toric_fibration to b3f06ed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants