Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#11599 closed enhancement (fixed)

Wrap fan morphism in toric morphism

Reported by: vbraun Owned by: AlexGhitza
Priority: major Milestone: sage-5.0
Component: algebraic geometry Keywords:
Cc: davideklund, fschulze, mmarco Merged in: sage-5.0.beta9
Authors: Volker Braun Reviewers: Andrey Novoseltsev
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by vbraun)

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:

Attachments (10)

trac_11599_no_circular_imports.py (24.2 KB) - added by vbraun 8 years ago.
Updated patch
trac_11599_no_circular_imports.patch (23.6 KB) - added by vbraun 8 years ago.
Rebased patch
trac_11599_rename_morphisms.2.patch (52.9 KB) - added by vbraun 8 years ago.
Rebased patch
trac_11599_toric_morphisms.patch (33.0 KB) - added by vbraun 8 years ago.
Rebased patch
trac_11599_homset_new_coercion_model.patch (148.0 KB) - added by vbraun 8 years ago.
Updated patch
trac_11599_rename_morphisms.patch (57.9 KB) - added by vbraun 8 years ago.
Updated patch
trac_11599_reviewer.patch (16.7 KB) - added by novoselt 8 years ago.
trac_11599_remove_class_suffix.patch (30.0 KB) - added by vbraun 8 years ago.
Initial patch
trac_11599_remaining_fixes.patch (15.9 KB) - added by vbraun 8 years ago.
Initial patch
trac_11599_numerical_noise.patch (1.5 KB) - added by vbraun 8 years ago.
Initial patch

Download all attachments as: .zip

Change History (63)

comment:1 Changed 8 years ago by novoselt

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

comment:2 Changed 8 years ago by novoselt

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

comment:3 Changed 8 years ago by vbraun

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 ;)

comment:4 Changed 8 years ago by novoselt

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.

comment:5 Changed 8 years ago by vbraun

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.

comment:6 Changed 8 years ago by vbraun

  • Description modified (diff)

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 :-)

comment:7 Changed 8 years ago by vbraun

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.

Changed 8 years ago by vbraun

Updated patch

comment:8 Changed 8 years ago by vbraun

  • Authors set to Volker Braun
  • Description modified (diff)
  • Status changed from new to needs_review

comment:9 Changed 8 years ago by novoselt

The first patch has wrong extension!

comment:10 Changed 8 years ago by vbraun

  • Description modified (diff)

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

comment:11 Changed 8 years ago by vbraun

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.

comment:12 Changed 8 years ago by novoselt

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.

comment:13 follow-up: Changed 8 years ago by 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.

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

comment:14 Changed 8 years ago by vbraun

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

comment:15 in reply to: ↑ 13 Changed 8 years ago by novoselt

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!

comment:16 Changed 8 years ago by novoselt

  • Reviewers set to Andrey Novoseltsev

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!

comment:17 Changed 8 years ago by zimmerma

  • Summary changed from Wrap fan moriphism in toric morphism to Wrap fan morphism in toric morphism

Changed 8 years ago by vbraun

Rebased patch

Changed 8 years ago by vbraun

Rebased patch

Changed 8 years ago by vbraun

Rebased patch

comment:18 Changed 8 years ago by vbraun

  • Cc davideklund fschulze mmarco added

Apply trac_11599_no_circular_imports.patch trac_11599_homset_new_coercion_model.patch trac_11599_rename_morphisms.patch trac_11599_toric_morphisms.patch

Rediffed because it conflicted with #11526

comment:19 follow-up: Changed 8 years ago by davideklund

Looking good.

There is a typo in trac_11599_homset_new_coercion_model.patch line 482: "...the heigh of the coordinates", should be "height".

comment:20 Changed 8 years ago by vbraun

Thanks, fixed.

comment:21 in reply to: ↑ 19 ; follow-up: Changed 8 years ago by novoselt

Replying to davideklund:

Looking good.

There is a typo in trac_11599_homset_new_coercion_model.patch line 482: "...the heigh of the coordinates", should be "height".

"Looking good" as in positive review to the second patch with the fixed typo?-)

comment:22 Changed 8 years ago by novoselt

Volker: regarding pickling issues and doctests marked "optional - pickling", is there already an open ticket for this? If not, should there be one?

comment:23 Changed 8 years ago by novoselt

Next questions: why AffineSpace_generic now inherits from AmbientSpace only and not AffineScheme as well?

comment:24 in reply to: ↑ 21 Changed 8 years ago by davideklund

Replying to novoselt:

Replying to davideklund:

Looking good.

There is a typo in trac_11599_homset_new_coercion_model.patch line 482: "...the heigh of the coordinates", should be "height".

"Looking good" as in positive review to the second patch with the fixed typo?-)

Sorry, I meant that it looks good in general, would need to go through it more carefully for a review. I was planning to look at this ticket though, I hope to contribute to the review of it (but it might take a while for me to get started since I will be traveling).

comment:25 Changed 8 years ago by novoselt

Thanks for a quick reply, David! I was going to review this ticket for a very embarrassingly long time (and have even boldly put myself as a reviewer)... My current plan is to go ahead and read through the patches during the break, but another set of eyes (and especially brains) would be very welcome at least for the general/non-toric part.

comment:26 Changed 8 years ago by vbraun

AffineScheme doesn't add much functionality to Scheme, which is the base for AmbientSpace. But I guess for completeness it should be listed amongst the parents. Its just my bias against multiple inheritance. Updated patch fixes it.

I haven't made a ticket about pickling. While we should strive for pickle-ability, there are many places that don't support it. The issue here is somewhat complicated interaction of Python and Cython, so I think one would have to make the pickler smarter in order to solve it...

Changed 8 years ago by vbraun

Updated patch

comment:27 Changed 8 years ago by novoselt

My concern is that people who do use elliptic curves may find out that their personal code does not work anymore with these patches...

Thanks for inheritance clarification!

In ambient_space.py: can NT's comment and related commented code be gone due to the changes? (So that it does not confuse those who read this file in the future.)

comment:28 Changed 8 years ago by novoselt

  • Description modified (diff)

With current patches:

SCORE sage/schemes/generic/homset.py: 85% (18 of 21)

Missing documentation:
	 * base_extend(self, R):


Missing doctests:
	 * is_SchemeHomset(H):
	 * _element_constructor_(self, *args, **kwds):


Possibly wrong (function name doesn't occur in doctests):
	 * _element_constructor_(self, x, check=True):
	 * _element_constructor_(self, *v, **kwds):
	 * _element_constructor_(self, *v, **kwds):
	 * _element_constructor_(self, *v, **kwds):

Would be nice to get to 100%.

Changed 8 years ago by vbraun

Updated patch

comment:29 Changed 8 years ago by vbraun

Added some more doctests to bring the whole directory to 95.4% doctest coverage ;-)

comment:30 Changed 8 years ago by novoselt

I am confused by SchemeHomset_generic.__call__ and its documentation. Why are things passed to Set_generic.__call__ and documentation should be in _call_ of derived classes? What exactly should be done by this particular class?

I have also started a reviewer patch on top of all others fixing little issues, it is not in its final form yet.

comment:31 Changed 8 years ago by novoselt

  • Description modified (diff)

comment:32 Changed 8 years ago by novoselt

What's the point in having SchemeHomset_points_projective_ring at all if all it does is implementing two non-implemented methods? Can't we just get rid of it?

comment:33 Changed 8 years ago by vbraun

Presumably somebody wanted to implement projective spaces over general rings but has not gotten around to doing it yet. I just kept what we have so that future generations know where to plug in the scheme morphism over rings. Also, some elliptic curve stuff derives from it. Again thats not really a good reason for keeping it but it might be useful in the future.

comment:34 Changed 8 years ago by novoselt

Done with the huge patch, all looks fine, little tweaks are in the reviewer patch.

Moving on to the remaining ones...

comment:35 Changed 8 years ago by novoselt

The renaming is great, but do we need _class on the end of _morphism_class, _homset_class, _point_class, _point_homset_class??? It seems to me that these functions return actual morphisms and homsets, not classes, so it is confusing (and it was especially confusing when they were undocumented). I propose getting rid of _class suffix.

There is also no consistency in "a word for homset": I think I saw "homset", "hom set", "hom-set", "Hom-set" and maybe other variants with capitalization-spacing-dashing. It would be nice to settle with a single one. Personally I prefer "Hom-set" with close second "homset". Or, perhaps, they should be referred to as "sets of morphisms".

Otherwise I am fine with the third patch, so only the fun part is left!

comment:36 Changed 8 years ago by novoselt

  • Status changed from needs_review to needs_work

For the last patch:

  1. In schemes.rst extra trailing spaces are added to one line (which I think was recently prohibited ;-)) and toric_morphism is added twice - probably it should be only once?
  2. In scheme.py it adds cached_method, but it was already imported, so this hunk should not be there.
  3. In toric_homset.py: I think that generic description should not be duplicated in the module docstring. On the other hand AUTHOURS, EXAMPLES and license sections should be added.
  4. __init__ in the beginning does not have a docstring at all. It would be nice also to add some clarifying comment what exactly is achieved by register_conversion there (although maybe I am supposed to know that?...)
  5. In the example of _element_constructor_ can we please not use Hom as the name for dP8.Hom(P2)? Hom is a top-level function and overwriting it is a bit confusing, especially since the example is quite long. How about just H instead?
  6. In _repr_defn of toric morphism lines like "Defined by sending the Rational polyhedral fan in 2-d lattice N to Rational polyhedral fan in 1-d lattice N." are constructed. "the" in front of domain but not codomain is a bit strange and I think we should remove it since fans are capitalized. Or put it in both places, but it makes strings longer ;-)
  7. In _homset_class of toric_variety.py the old doctest is deleted and it is actually no longer working. Without direct calls to private functions, H([z0, z1, z0, z3]) is also not working, which bugs me quite a bit. The traceback goes to NotImplementedError and includes Set_generic.__call__ which I have complained a bit in http://trac.sagemath.org/sage_trac/ticket/11599#comment:30.

These are all the comments that I have, will upload new reviewer patch shortly.

Changed 8 years ago by novoselt

Changed 8 years ago by vbraun

Initial patch

Changed 8 years ago by vbraun

Initial patch

comment:37 Changed 8 years ago by vbraun

  • Description modified (diff)
  • Status changed from needs_work to needs_review

I renamed the method names to remove the _class suffix, this is the first new patch.

The final patch should deal with all of your remaining suggestions.

comment:38 Changed 8 years ago by novoselt

Thank you!

What is the purpose of fan in algebraic_scheme.py in the very last patch? I think it is confusing to return for a subscheme the fan of the ambient space, especially if the subscheme happens to be a toric variety itself and then can potentially have its own associated fan of a smaller dimension.

comment:39 Changed 8 years ago by vbraun

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.

comment:40 Changed 8 years ago by novoselt

  • Status changed from needs_review to positive_review

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

comment:41 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

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)
**********************************************************************

comment:42 Changed 8 years ago by novoselt

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

Changed 8 years ago by vbraun

Initial patch

comment:43 Changed 8 years ago by vbraun

  • Description modified (diff)
  • Status changed from needs_work to needs_review

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

comment:44 Changed 8 years ago by novoselt

  • Status changed from needs_review to positive_review

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

comment:45 Changed 8 years ago by jdemeyer

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

comment:46 Changed 8 years ago by davidloeffler

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)

comment:47 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.0.beta9
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:48 Changed 8 years ago by hivert

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

comment:49 Changed 8 years ago by jdemeyer

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

comment:50 Changed 8 years ago by jdemeyer

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

comment:51 follow-up: Changed 7 years ago by cremona

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 http://trac.sagemath.org/sage_trac/ticket/11599

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

comment:52 Changed 7 years ago by cremona

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.

comment:53 in reply to: ↑ 51 Changed 7 years ago by jdemeyer

Replying to cremona:

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

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

Note: See TracTickets for help on using tickets.