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:

Status badges

Description (last modified by novoselt)

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)

trac_9839_add_dual_cone_computation.patch (6.8 KB) - added by novoselt 11 years ago.
trac_9839_reviewer.patch (6.9 KB) - added by vbraun 11 years ago.
Updated patch
trac_9839_reviewer.2.patch (6.9 KB) - added by novoselt 11 years ago.
Adjusted version

Download all attachments as: .zip

Change History (11)

Changed 11 years ago by novoselt

comment:1 Changed 11 years ago by novoselt

  • Authors set to Andrey Novoseltsev
  • Cc vbraun added
  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 11 years ago by vbraun

I don't like that your lattice now needs to have self.dual() implemented:

sage: c = Cone([(1,0), [0,1]], lattice = ZZ^2)
sage: c.facet_normals()
AttributeError                            Traceback (most recent call last)

/home/vbraun/opt/sage-4.5.2/devel/sage-main/<ipython console> in <module>()

/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/geometry/cone.pyc in facet_normals(self)
   1808         """
   1809         if "_facet_normals" not in self.__dict__:
-> 1810             M = self.lattice().dual()
   1811             P = self.lattice_polytope()
   1812             rotate_lifts = not self.is_strictly_convex()

/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/structure/ in sage.structure.parent.Parent.__getattr__ (sage/structure/parent.c:5311)()

/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/structure/ in sage.structure.parent.getattr_from_other_class (sage/structure/parent.c:2757)()

/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/structure/ in sage.structure.parent.raise_attribute_error (sage/structure/parent.c:2629)()

AttributeError: 'FreeModule_ambient_pid_with_category' object has no attribute 'dual'

This essentially limits you to ToricLattices now. I think it would be good for factet_normals() to use the same lattice otherwise, so ZZ^n works as well.

The rest looks good!

comment:3 Changed 11 years ago by vbraun

  • 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 novoselt

Great additions for the dual lattice and documentation clarifications!

Just a couple of suggestions: can we

  • name the method dual_lattice instead of lattice_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?

Changed 11 years ago by vbraun

Updated patch

comment:5 Changed 11 years ago by vbraun

Renamed to dual_lattice and added documentation.

Changed 11 years ago by novoselt

Adjusted version

comment:6 Changed 11 years ago by novoselt

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 vbraun

  • Status changed from needs_review to positive_review

Looks good. Positive review.

For the release coordinator: Apply

  1. trac_9839_add_dual_cone_computation.patch
  2. trac_9839_reviewer.2.patch

comment:8 Changed 11 years ago by mpatel

  • Merged in set to sage-4.6.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.