Opened 10 years ago
Last modified 9 years ago
#11599 closed enhancement
Wrap fan morphism in toric morphism — at Version 31
Reported by: | vbraun | Owned by: | AlexGhitza |
---|---|---|---|
Priority: | major | Milestone: | sage-5.0 |
Component: | algebraic geometry | Keywords: | |
Cc: | davideklund, fschulze, mmarco | Merged in: | |
Authors: | Volker Braun | Reviewers: | Andrey Novoseltsev |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
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:
Change History (37)
comment:1 Changed 10 years ago by
comment:2 Changed 10 years ago by
P.S. Ordered dictionaries seem very appropriate for this approach, but we need to upgrade Python for them.
comment:3 Changed 10 years ago by
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 10 years ago by
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 10 years ago by
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 10 years ago by
- 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 10 years ago by
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.
comment:8 Changed 10 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:9 Changed 10 years ago by
The first patch has wrong extension!
comment:10 Changed 10 years ago by
- Description modified (diff)
Oops, I clearly never read past the first letter of the extension ;-)
comment:11 Changed 10 years ago by
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 10 years ago by
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: ↓ 15 Changed 10 years ago by
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 10 years ago by
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 10 years ago by
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 10 years ago by
- 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 10 years ago by
- Summary changed from Wrap fan moriphism in toric morphism to Wrap fan morphism in toric morphism
comment:18 Changed 10 years ago by
- 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: ↓ 21 Changed 10 years ago by
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 10 years ago by
Thanks, fixed.
comment:21 in reply to: ↑ 19 ; follow-up: ↓ 24 Changed 10 years ago by
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 10 years ago by
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 10 years ago by
Next questions: why AffineSpace_generic
now inherits from AmbientSpace
only and not AffineScheme
as well?
comment:24 in reply to: ↑ 21 Changed 10 years ago by
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 10 years ago by
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 10 years ago by
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...
comment:27 Changed 10 years ago by
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 10 years ago by
- 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%.
comment:29 Changed 10 years ago by
Added some more doctests to bring the whole directory to 95.4% doctest coverage ;-)
comment:30 Changed 10 years ago by
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 10 years ago by
- Description modified (diff)
We should. I have been using the following code so far:
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