Opened 5 years ago
Closed 5 years ago
#11200 closed enhancement (fixed)
Add fibration check to FanMorphism
Reported by: | novoselt | Owned by: | mhampton |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7.1 |
Component: | geometry | Keywords: | toric |
Cc: | vbraun | Merged in: | sage-4.7.1.alpha3 |
Authors: | Andrey Novoseltsev | Reviewers: | Volker Braun |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #10140, #10882 | Stopgaps: |
Description (last modified by novoselt)
The main point of this ticket was to add FanMorphism.is_fibration method, but along the way injectivity, surjectivity, and splitting were added as well.
I have also added several related methods and adjusted containment/embedding methods of fans to accommodate sublattices and make their behaviour more uniform: now cone containment check is an attempt to embed the given cone into the fan.
I have also started using @cached_method decorator since it is convenient and its speed and documentation issues should be resolved in the near future. Note, however, that some methods of fans still should use manual cache since they need to preprocess arguments first.
Attachments (1)
Change History (23)
comment:1 Changed 5 years ago by novoselt
- Status changed from new to needs_review
comment:2 Changed 5 years ago by novoselt
- Dependencies set to #10140, #10882
- Description modified (diff)
comment:3 Changed 5 years ago by vbraun
- Reviewers set to Volker Braun
I like the functionality, but you need to check surjectivity of the map of toric varieties, not just the map of vector spaces. The is_surjective() method should do that but is broken:
sage: P1 = toric_varieties.P1() sage: A1 = toric_varieties.A1() sage: phi = FanMorphism(matrix([[1]]), A1.fan(), P1.fan()) sage: phi.is_fibration() True sage: phi.is_surjective() True
But thats neither a fibration nor a surjective map.
comment:4 Changed 5 years ago by vbraun
- Status changed from needs_review to needs_work
comment:5 Changed 5 years ago by novoselt
Hmmm, I thought that it does check support surjection, will look into it.
As for is_surjective, it is wrong because it is inherited. I will add an appropriate toric implementation to this patch.
comment:6 Changed 5 years ago by novoselt
- Status changed from needs_work to needs_info
Hi Volker,
I fixed the mistake you have found and wrote is_surjective.
While I am at it, I also started writing is_injective, but it is a bit trickier: we want the lattice map to have index one in the saturation of its image (in Sage, conveniently, self.matrix().index_in_saturation() == 1) and for every (primitive) cone tau mapped to sigma the dimension of tau must be the same as the dimension of the intersection of sigma with the image lattice of the lattice map. To make this easier and potentially simplify life in the future with computing fibers of generic fan/toric morphisms, I am thinking of adding the following methods:
- image_lattice to be the sublattice generated by the image of the domain lattice.
- image_fan to be the intersection of the codomain fan with the image lattice.
- image_restriction to be the fan morphism from the original domain fan to the newly defined image fan.
Question: how should we name these methods? It seems to me that image_lattice fits nicely with kernel_lattice, but two others may be quite confusing/misleading, i.e. image_fan may refer to the image of the domain fan in the image lattice. One possible solution is not to implement such a method at all as it may be accessed via
phi.image_restriction().codomain_fan()
Any thoughts on this and the name for the image_restriction map?
comment:7 Changed 5 years ago by novoselt
Update: actually, just image already does what I called image_lattice above, there is also kernel that seems to do kernel_lattice job, so maybe image/kernel are sufficient. Then the only issue is image_restriction!
comment:8 Changed 5 years ago by novoselt
And there is also restrict_codomain, similar in spirit, although I probably want to cache this special "natural" restriction.
comment:9 Changed 5 years ago by vbraun
Since our naming theme is to use method() for the inherited method on vector spaces and method_fan() for the induced function of the fan, how about restrict_domain_fan() and restrict_codomain_fan(). The restriction to the image would then be
phi.restrict_codomain_fan( phi.image_fan() )
For the restriction of the vector space map you have to use this construction as well, I think; as far as I can tell there is no method that restricts to the image in one go.
Also, I don't think image_fan() is overly confusing. After all, the fan is a collection of cones over \QQ.
For injectivity you need to check that the index of each domain cone is one (Def 2.1.7 in HLY). Are you going to override the FanMorphism.index() method? I would have thought that this is a logical place to put this functionality.
comment:10 Changed 5 years ago by novoselt
Yes, phi.index(sigma) is the way to go, I think, but having these indices equal to 1 is not enough (and they are all 1 if the linear map has index one in its image saturation). We also need "relative stars" to be trivial, so that in the notation of Proposition 2.1.4 in HLY \tld{O}_\sigma is a 1-to-1 cover and F_\sigma^c is a singleton.
Going back to names, I will try to be more careful in the future with checking inherited methods, but it seems to me now that
- kernel_lattice should be removed since kernel computes exactly the same thing using generic code.
- image_lattice should not be implemented since phi.image().saturation() does what I want and plane image() is also of interest in the toric setting. (Currently, saturation will not return a toric sublattice, but it is easy to fix.)
For the factoring of the fan morphism phi: Sigma ---> Sigma' we (always) have the following sequence of fan maps:
Sigma0 --0--> Sigma --1--> Sigma1 --2--> Sigma2 --3--> Sigma'
where Sigma0 is the kernel fan, Sigma1 is phi(Sigma) living in phi(N_RR) \cap N', and Sigma2 is phi(N_RR) \cap Sigma'. Map 1 is easy to construct as
FanMorphism(phi.matrix(), phi.domain_fan(), phi.image().saturation())
Maps 2 and 3 are basically given by identity matrix but there is currently no "one line way" to construct Sigma2 which I want to add. Now what I was referring to, is that both Sigma1 and Sigma2 are natural candidates to be called image_fan since they are fans in the image of phi and perhaps Sigma1 is more natural since it is literally the image fan of Sigma under phi.
Since "general" restrictions of phi to other fans can be easily done using FanMorphism constructor directly, I am not sure there is any need in adding special methods to restrict (co)domain to a provided fan. So I think I am in favor of adding only, say, restrict_to_image() with no arguments which will be the composition of maps 1 and 2 in the diagram. The main job of this function will be, of course, efficient construction of Sigma2 and once it is done all maps of the sequence can be easily constructed if needed.
Summary of the proposal:
- remove kernel_lattice and use inherited kernel instead;
- tweak saturation to make phi.image().saturation() to return a toric sublattice;
- add restrict_to_image returning the map Sigma ---> Sigma2;
- let phi.index(sigma) take a cone as an argument and return its index in the sense of HLY.
Sounds good?
comment:11 Changed 5 years ago by novoselt
- Work issues set to sublattice duals and action
Update: 1 and 2 are easy and done, 3 is in principle also easy and done, but it does not yet work due to sublattice handling. So I am working on improving it. Ideally I want to be able to do in sublattices and quotient lattices everything that can be done in the ambient ones. Several steps in this direction were already done, this patch will make a few more ;-)
comment:12 Changed 5 years ago by novoselt
- Description modified (diff)
- Status changed from needs_info to needs_review
- Work issues sublattice duals and action deleted
OK, I propose this version for inclusion. I did not add index(cone) functionality as it is not necessary for the current methods and it would make sense only for those morphisms whose vector space map is surjective (there should be perhaps another is_xxx method to test for this, but I am not sure how to call it - is_dense?). Besides, someone else is likely to work on further description of fan morphism fibers and it will be a nice exercise to add cone indices ;-)
I made a small change to free_module, but didn't yet test consequences on the whole library. I'll do it today and will fix any problems if there are any.
comment:13 Changed 5 years ago by novoselt
Fixed a couple of typos in the documentation and removed "not tested" comment in the kernel_fan as it is now possible to construct its cone lattice.
comment:14 Changed 5 years ago by novoselt
While patchbot made a complete streetlight thinking that this patch get worse and worse, direct testing shows that all long tests pass on top of sage-4.7.rc2 with dependencies applied!
comment:15 Changed 5 years ago by vbraun
Looks all good to me.
Just a minor nitpick: I think that is_splitting() is very unintuitive. Does anyone besides HLY use that notation? How about we rename it to is_bundle() or at least make an alias?
comment:16 Changed 5 years ago by novoselt
- Status changed from needs_review to needs_work
- Work issues set to splitting ---> bundle
That was actually from "Toric Varieties" book. But I like is_bundle more, so I'll change it. Thanks for the suggestion!
comment:17 Changed 5 years ago by novoselt
- Status changed from needs_work to needs_review
- Work issues splitting ---> bundle deleted
Done!
comment:18 Changed 5 years ago by vbraun
- Status changed from needs_review to positive_review
Nice!
The patch buildbot is confused about which patches from the dependency #10140 need to be applied. I tried to help the patch buildbot to figure it out but without success. All doctest pass on my machine.
comment:19 Changed 5 years ago by vbraun
- Dependencies changed from #10140, #10882 to #10140, #10882, #9128
The documentation makes use of :trac:... sphinx tags and therefore depends on #9128.
Changed 5 years ago by novoselt
comment:20 Changed 5 years ago by novoselt
- Dependencies changed from #10140, #10882, #9128 to #10140, #10882
Not anymore :-(
#9128 has been around for a while and it has further dependencies, I'd rather not delay this ticket till they all are resolved...
comment:21 Changed 5 years ago by vbraun
Great, ready to be included in any 4.7.1.alpha now!
comment:22 Changed 5 years ago by jdemeyer
- Merged in set to sage-4.7.1.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
Rebased on top of new #10882.