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

Wrap fan morphism in toric morphism #11599

Closed
vbraun opened this issue Jul 14, 2011 · 69 comments
Closed

Wrap fan morphism in toric morphism #11599

vbraun opened this issue Jul 14, 2011 · 69 comments

Comments

@vbraun
Copy link
Member

vbraun commented Jul 14, 2011

Since we have the very nice fan morphism class, we should use it to define toric morphisms of toric varieties.

A big part of the patch is porting the scheme morphisms / hom sets to new-style parents and coercion. Categories should be better, too. Fixes #7946 and #6810 as a side effect.

The first two patches bring some sanity to the scheme morphisms. The 3rd patch changes names of methods/classes to something more reasonable and adds documentation. The 4th patch actually adds toric morphisms defined by polynomials or fan morphisms.

Apply:

CC: @sagetrac-davideklund @sagetrac-fschulze @miguelmarco

Component: algebraic geometry

Author: Volker Braun

Reviewer: Andrey Novoseltsev

Merged: sage-5.0.beta9

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

@novoselt
Copy link
Member

comment:1

We should. I have been using the following code so far:

def toric_morphism_list(phi, X, Y):
    r"""
    Given a fan morphism phi from X.fan() -> Y.fan(), return the list representation of the corresponding toric morphism.
    """
    x = [SR.var(x_i, domain="positive") for x_i in X.coordinate_ring().variable_names()]
    result = [1] * Y.fan().nrays()
    for rho, x_rho in zip(X.fan(1), x):
        sigma_prime = phi.image_cone(rho)
        degrees = sigma_prime.ray_matrix() \ phi(rho.ray(0))
        for i, d in zip(sigma_prime.ambient_ray_indices(), degrees):
            result[i] *= x_rho^d
    return result

def toric_morphism_dictionary(phi, X, Y):
    r"""
    Given a fan morphism phi from X.fan() -> Y.fan(), return the dictionary representation of the corresponding toric morphism.
    """
    x = [SR.var(x_i, domain="positive") for x_i in X.coordinate_ring().variable_names()]
    y = [SR.var(y_i, domain="positive") for y_i in Y.coordinate_ring().variable_names()]
    result = dict((y_i, 1) for y_i in y)
    for rho, x_rho in zip(X.fan(1), x):
        sigma_prime = phi.image_cone(rho)
        degrees = sigma_prime.ray_matrix() \ phi(rho.ray(0))
        for i, d in zip(sigma_prime.ambient_ray_indices(), degrees):
            result[y[i]] *= x_rho^d
    return result

Using the dictionary representation it is quite easy to compute pullbacks, the problem here is that the underlying map of total coordinate rings is not a ring homomorphism, since it is likely to involve roots. The following paper may be useful for "correct and general" implementation:
http://arxiv.org/abs/1004.4924

@novoselt
Copy link
Member

comment:2

P.S. Ordered dictionaries seem very appropriate for this approach, but we need to upgrade Python for them.

@vbraun
Copy link
Member Author

vbraun commented Jul 16, 2011

comment:3

Thanks for pointing out the reference. Its fairly obvious that one has to use roots to write the maps but still its good to see that somebody worked out all the details. Though from a quick browse it seems like they don't elaborate on the relation with fan morphisms. E.g. the embedding of the obit closure can be written as a polynomial map in homogeneous coordinates but is not a toric morphism (given by a fan morphism).

My plan is to implement maps by homogeneous coordinate polynomials and maps by fan morphisms separately, with conversion methods from one to the other if it exists.

Eventually we should also have maps involving roots. I'm not sure how we should implement them; Just using symbolic ring variables would be simple but not play nice with compositions. At one point it would be good to write our own "Homogeneous coordinate ring" class that knows about the homogeneous rescalings. This would then allow for fractional powers in some nicer way. But I'll leave it for another ticket ;)

@novoselt
Copy link
Member

comment:4

They don't elaborate the relation with fan morphisms because they consider arbitrary maps of toric varieties as varieties, including those that don't care about toric structure at all. In particular it is applicable to equivariant morphisms and orbit inclusions. And even in these cases it is necessary to use roots if one of the varieties is not smooth. E.g. a resolution of a singular variety would correspond to the identity map of lattices, but would involve roots in homogeneous coordinates.

@vbraun
Copy link
Member Author

vbraun commented Jul 16, 2011

comment:5

I know. But without roots you can still have homogeneous polynomial maps and toric morphisms. Neither of the two is contained in the other. So I'm planning on implementing toric (equivariant) morphism separately.

@vbraun

This comment has been minimized.

@vbraun
Copy link
Member Author

vbraun commented Jul 19, 2011

comment:6

Ok so far no new functionality. But I think now the groundwork is done and we can actually do some work on top of it. I'm sorry for the giant patch :-)

@vbraun
Copy link
Member Author

vbraun commented Jul 19, 2011

comment:7

Pickling is broken for some complex object like EllipticCurveTorsionSubgroup that contain circular self-references. This is a bug in unpickling the category framework, and will be addressed elsewhere. I've marked the offending unpickling doctests with # optional - pickling so we can fix them later.

@vbraun
Copy link
Member Author

vbraun commented Aug 11, 2011

Updated patch

@vbraun
Copy link
Member Author

vbraun commented Aug 11, 2011

Author: Volker Braun

@vbraun

This comment has been minimized.

@vbraun
Copy link
Member Author

vbraun commented Aug 11, 2011

comment:8

Attachment: trac_11599_no_circular_imports.py.gz

@novoselt
Copy link
Member

comment:9

The first patch has wrong extension!

@vbraun

This comment has been minimized.

@vbraun
Copy link
Member Author

vbraun commented Aug 12, 2011

comment:10

Oops, I clearly never read past the first letter of the extension ;-)

@vbraun
Copy link
Member Author

vbraun commented Aug 13, 2011

comment:11

I've noticed that there are various issues with torus factors, that is, not full-dimensional fans. While you can easily ignore the torus factor when dealing with a single toric variety, we need to be more careful when we consider morphisms. In any case, this needs some more work and I propose to do that elsewhere.

@novoselt
Copy link
Member

comment:12

Yes, I had this problem when I was dealing with charts corresponding to non-full-dimensional cones. We need to keep the information about directions corresponding to torus factor coordinates, standard basis vectors are not always the best choice. Magma calls these directions "virtual rays" but I am not fond of this name. In any case, I was going to work on it but after having #11400 in place to have a uniform access to representations of different types of rays.

@vbraun
Copy link
Member Author

vbraun commented Aug 13, 2011

comment:13

I thought we might be able to add the torus factor rays at the end of fan.rays(), so that we always have a 1-1 correspondence between rays and homogeneous variables. The list of 1-cones starts with the rays but does not include the torus factor rays. But I haven't tried this to see how it will work out.

Does this mean that you want me to review #11400 now? ;-)

@vbraun
Copy link
Member Author

vbraun commented Aug 13, 2011

comment:14

I just noticed that we still haven't merged #10793. Of course I had some matrices accidentally transposed...

@novoselt
Copy link
Member

comment:15

Replying to @vbraun:

I thought we might be able to add the torus factor rays at the end of fan.rays(), so that we always have a 1-1 correspondence between rays and homogeneous variables. The list of 1-cones starts with the rays but does not include the torus factor rays. But I haven't tried this to see how it will work out.

That's an interesting idea, but we should be very careful if we implement it: there may be places where the rays of the fan are supposed to be "honest" and I am not quite sure how to search for such places. But maybe I just worry too much.

Does this mean that you want me to review #11400 now? ;-)

I certainly would not mind ;-) And thanks for reviewing #10793!

@novoselt
Copy link
Member

Reviewer: Andrey Novoseltsev

@novoselt
Copy link
Member

comment:16

For the record: the first patch looks totally fine to me. Also all long doctests pass with arbitrary number of patches applied, so they could be merged one-by-one. I definitely would like to slowly go over the code of the last patch myself, but if someone wants to approve 2 and 3 - you are very welcome!

@zimmermann6 zimmermann6 changed the title Wrap fan moriphism in toric morphism Wrap fan morphism in toric morphism Sep 12, 2011
@vbraun
Copy link
Member Author

vbraun commented Dec 16, 2011

Rebased patch

@vbraun
Copy link
Member Author

vbraun commented Feb 19, 2012

comment:39

If you want to construct a toric morphism from an algebraic scheme then the algebraic scheme needs to have a fan() method. Of course one could change he toric morphism to work around that, but after some thinking I thought it is best to do it. A toric subscheme is in general not a toric variety, so there shouldn't be any confusion.

@novoselt
Copy link
Member

comment:40

Well, looks good, hopefully it will make its way into Sage-5.0!

@jdemeyer
Copy link

comment:41

There is numerical noise on various systems (including ia64 and OSX 10.4 PPC):

**********************************************************************
File "/home/buildbot/build/sage/cleo-1/cleo_full/build/sage-5.0.beta6/devel/sage-main/sage/schemes/generic/morphism.py", line 1292:
    sage: P(2+I, 3-I, 4+5*I)
Expected:
    (0.317073170732 - 0.146341463415*I : 0.170731707317 - 0.463414634146*I : 1.0)
Got:
    (0.317073170732 - 0.146341463415*I : 0.170731707317 - 0.463414634146*I : 1.0 - 1.73387706291e-17*I)
**********************************************************************

@novoselt
Copy link
Member

novoselt commented Mar 6, 2012

comment:42

So - what would be the correct way to deal with such noise?..

@vbraun
Copy link
Member Author

vbraun commented Mar 6, 2012

Initial patch

@vbraun

This comment has been minimized.

@vbraun
Copy link
Member Author

vbraun commented Mar 6, 2012

comment:43

Attachment: trac_11599_numerical_noise.patch.gz

I propose to just set the homogeneous variable to 1 instead of dividing by itself...

@novoselt
Copy link
Member

novoselt commented Mar 6, 2012

comment:44

Seems reasonable to me and passes all tests on Sage-5.0.beta7 (although I cannot test on those platforms).

@jdemeyer
Copy link

comment:45

For your information:
this patch conflicts with #715 (in the sense that a doctest from #715 fails with this patch applied).

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 12, 2012

comment:46

Apply trac_11599_no_circular_imports.patch, trac_11599_homset_new_coercion_model.patch, trac_11599_rename_morphisms.patch, trac_11599_toric_morphisms.patch, trac_11599_reviewer.patch, trac_11599_remove_class_suffix.patch, trac_11599_remaining_fixes.patch, trac_11599_numerical_noise.patch

(for the patchbot, which is failing #12544 because it tries to apply the patches here in the wrong order)

@jdemeyer
Copy link

Merged: sage-5.0.beta9

@hivert
Copy link

hivert commented Apr 10, 2012

comment:48

Hi there,

It's cool to see thinks moving forward and the doc getting better. However
communication between developers could be better too. In this ticket, there is
a big chunk fixing the doc of sage/structure/factory.pyx. In my ticket,
improving classcall and UniqueRepresentation, I also discovered
that sage/structure/factory.pyx wasn't in the doc and that its doc was
broken. I didn't find any ticket that seems to fix it so I decided to go ahead
and spend on hour improving it... For nothing except creating a conflict.

Next time, please make such unrelated doctest improvements in a separate
ticket, and advertise them on trac!

Thanks,

Grumpy Florent

@jdemeyer
Copy link

comment:49

The patches here cause a serious slow-down in parts of the elliptic curve code, see #12853.

@jdemeyer
Copy link

comment:50

More precisely, the patch attachment: trac_11599_homset_new_coercion_model.patch causes the slow-down.

@JohnCremona
Copy link
Member

comment:51

In trac_11599_homset_new_coercion_model.patch a comment is added in line 529 (now line 531) of sage/schemes/elliptic_curve/ell_curve_isogeny, which now reads

        sage: phi == loads(dumps(phi))   # optional - pickling https://github.com/sagemath/sage-prod/issues/11599

and now fails if testing is done with the --optional flag. Should "optional" be changed to "not tested" here until it is fixed?

@JohnCremona
Copy link
Member

comment:52

PS Same on line 158 of ell_torsion.py. As I understand it the tag #optional should be acompanied by the name of an optional spkg.

@jdemeyer
Copy link

comment:53

Replying to @JohnCremona:

Should "optional" be changed to "not tested" here until it is fixed?

Even better: "known bug", see #14362.

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

6 participants