Opened 11 years ago
Closed 11 years ago
#9839 closed enhancement (fixed)
Add dual cone computation
Reported by: | novoselt | Owned by: | mhampton |
---|---|---|---|
Priority: | major | Milestone: | sage-4.6 |
Component: | geometry | Keywords: | |
Cc: | vbraun | Merged in: | sage-4.6.alpha1 |
Authors: | Andrey Novoseltsev | Reviewers: | Volker Braun |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
This patch allows computing dual cones, including non-strictly convex and non-full-dimensional cases.
The actual work is done in facet_normals
which now works for non-strictly convex cones as well. The method base_extend
for quotient lattices was added during one of the implementation attempts and I left it for future use as well (the hope was to create cones in quotient lattices, but it does not work yet).
There is still a dimension 6 limitation stemming from PALP for computing duals and facet normals. This is still our best option for low dimension, but perhaps it would be nice if facet_normals
caught the exception when PALP does not work and used polyhedra module in this case. Then computing the dual cone and face lattices should work automatically.
See #9604 for dependencies.
Attachments (3)
Change History (11)
Changed 11 years ago by
comment:1 Changed 11 years ago by
- Cc vbraun added
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 11 years ago by
comment:3 Changed 11 years ago by
- Reviewers set to Volker Braun
The absence of dual()
also annoyed me in the divisor class stuff which is built on top of ZZ^n
... So here is a patch. I added a new cone.lattice_dual()
method which returns the dual toric lattice (if possible) or the original lattice (if not possible).
I'm fine with the current state of affairs, so feel free to set positive review if its ok with you.
comment:4 Changed 11 years ago by
Great additions for the dual lattice and documentation clarifications!
Just a couple of suggestions: can we
- name the method
dual_lattice
instead oflattice_dual
and - add to the documentation of this method itself an explanation that "dual lattice" is the same as "just lattice" if it does not have
dual()
method?
comment:5 Changed 11 years ago by
Renamed to dual_lattice
and added documentation.
comment:6 Changed 11 years ago by
Thank you! I caught a broken link and changed a little bit documentation in lines 1743-1749 of the new patch. If it looks OK, please switch to positive review!
comment:7 Changed 11 years ago by
- Status changed from needs_review to positive_review
Looks good. Positive review.
For the release coordinator: Apply
trac_9839_add_dual_cone_computation.patch
trac_9839_reviewer.2.patch
comment:8 Changed 11 years ago by
- Merged in set to sage-4.6.alpha1
- Resolution set to fixed
- Status changed from positive_review to closed
I don't like that your lattice now needs to have
self.dual()
implemented:This essentially limits you to
ToricLattice
s now. I think it would be good forfactet_normals()
to use the same lattice otherwise, soZZ^n
works as well.The rest looks good!