Opened 7 years ago
Closed 6 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 )
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 7 years ago by
- Status changed from new to needs_review
comment:2 Changed 6 years ago by
- Dependencies set to #10140, #10882
- Description modified (diff)
comment:3 Changed 6 years ago by
- 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 6 years ago by
- Status changed from needs_review to needs_work
comment:5 Changed 6 years ago by
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 6 years ago by
- 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 6 years ago by
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 6 years ago by
And there is also restrict_codomain
, similar in spirit, although I probably want to cache this special "natural" restriction.
comment:9 Changed 6 years ago by
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 6 years ago by
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 sincekernel
computes exactly the same thing using generic code.image_lattice
should not be implemented sincephi.image().saturation()
does what I want and planeimage()
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 inheritedkernel
instead; - tweak
saturation
to makephi.image().saturation()
to return a toric sublattice; - add
restrict_to_image
returning the mapSigma ---> 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 6 years ago by
- 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 6 years ago by
- 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 6 years ago by
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 6 years ago by
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 6 years ago by
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 6 years ago by
- 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 6 years ago by
- Status changed from needs_work to needs_review
- Work issues splitting ---> bundle deleted
Done!
comment:18 Changed 6 years ago by
- 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 6 years ago by
- Dependencies changed from #10140, #10882 to #10140, #10882, #9128
The documentation makes use of :trac:...
sphinx tags and therefore depends on #9128.
Changed 6 years ago by
comment:20 Changed 6 years ago by
- 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 6 years ago by
Great, ready to be included in any 4.7.1.alpha now!
comment:22 Changed 6 years ago by
- 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.