Opened 11 years ago

Closed 11 years ago

#10336 closed enhancement (fixed)

Cannot plot non-strictly convex cones

Reported by: vbraun Owned by: novoselt
Priority: major Milestone: sage-4.6.2
Component: algebraic geometry Keywords:
Cc: novoselt Merged in: sage-4.6.2.alpha2
Authors: Andrey Novoseltsev Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges


Plotting cones relies on the cone's face lattice:

sage: halfspace = Cone([(1,0,0), (0,1,0), (-1,-1,0), (0,0,1)])
sage: halfspace.plot()
NotImplementedError                       Traceback (most recent call last)

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

/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/geometry/cone.pyc in plot(self, **options)
   2623         if self.dim() >= 2:
   2624             # Modify wall labels to match the ambient cone or fan too.
-> 2625             walls = self.faces(2)
   2626             try:
   2627                 ambient_walls = self.ambient().faces(2)

/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/geometry/cone.pyc in faces(self, dim, codim)
   2015         if "_faces" not in self.__dict__:
   2016             self._faces = tuple(self._sort_faces(e.element for e in level)
-> 2017                                 for level in self.face_lattice().level_sets())
   2018             # To avoid duplication and ensure order consistency
   2019             if self.dim() > 0:

/home/vbraun/Sage/sage/local/lib/python2.6/site-packages/sage/geometry/cone.pyc in face_lattice(self)
   1854                 # We need to compute face lattice on our own
   1855                 if not self.is_strictly_convex():
-> 1856                     raise NotImplementedError("face lattice is currently "
   1857                                 "implemented only for strictly convex cones!")
   1858                 def ConeFace(rays, facets):

NotImplementedError: face lattice is currently implemented only for strictly convex cones!

Is there any reason for why we can't compute the face lattice of a non-strictly convex cone? If there is, we might want to still plot something reasonable in that case...

Attachments (1)

trac_10336_plot_non_strictly_convex_cones.patch (19.9 KB) - added by novoselt 11 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 11 years ago by novoselt

  • Owner changed from AlexGhitza to novoselt
  • Type changed from defect to enhancement

Originally there was no code to compute face lattices of non-strictly convex cones, but with your recent modifications it will be easy to fix.

Plotting code that I wrote will not immediately work for such cases anyway, but again it should not be terribly difficult to fix, I just was more concerned with plotting fans.

Will work on it!

P.S. Since it was due to NotImplemented, I consider this ticket an enhancement ;-)

comment:2 follow-up: Changed 11 years ago by novoselt

I am thinking of:

  • allowing cone.faces(d) to accept any d and return an empty tuple if there are no faces of dimension d;
  • returning a tuple of non-empty tuples as the output of cone.faces(), i.e. for a half-space it will be just ((plane,), (half-space,)) instead of ((), (), (plane,), (half-space,)) where the d-th tuple contains faces of dimension d.


comment:3 Changed 11 years ago by novoselt

  • Status changed from new to needs_work
  • Work issues set to fix faces and add plotting

I attached the first development version. face_lattice should work for any cone now and there are a few small corrections in other places. Definitely needs more work!-)

comment:4 in reply to: ↑ 2 Changed 11 years ago by vbraun

Replying to novoselt:

I am thinking of:

  • allowing cone.faces(d) to accept any d and return an empty tuple if there are no faces of dimension d;
  • returning a tuple of non-empty tuples as the output of cone.faces()

both sound good to me!

comment:5 Changed 11 years ago by novoselt

  • Milestone changed from sage-5.0 to sage-4.6.2
  • Work issues changed from fix faces and add plotting to add plotting

Great! Done! Refreshed patch should deal fine with faces in all cases, all old doctests pass and I added a couple for non-strictly convex cones - there was an issue originally for "totally non-strictly conex" cones like a plane, that's why there is DiGraph(1) in Hasse_diagram.

Regarding the plots. Let's take, e.g. a half-plane generated by vectors (1,0), (1,1), (0,1), (-1,0). I propose that its plot will look exactly like the plot of the fan with these generators except that two middle vectors will be plotted as generators, but there will be no rays in their direction extending to the boundary of the plot (labels still will be places on the boundary out of colored regions). The label for the half-plane itself will be randomly placed between two consecutive generators - this way it will be inside and will not get crossed by one of the internal generators.

comment:6 Changed 11 years ago by novoselt

  • Work issues changed from add plotting to wall labels positioning and round mode

OK, it remains to make the default "round" mode work and reposition labels of non-strictly convex 2-d walls.

comment:7 Changed 11 years ago by novoselt

  • Authors set to Andrey Novoseltsev
  • Status changed from needs_work to needs_review
  • Work issues wall labels positioning and round mode deleted

Now plotting should work in all modes.

Wall labels will be positioned between two generators that generate the plane of the wall. This will result in OK placement for automatically adjusted generators (i.e. when half-planes are generated by 3 rays and planes by 4). If the user has forced (via check=False option) other generators, they can potentially cross labels, in which case this user should place labels manually.

I have switched toric_plotter to using RDF instead of RR since the first seems to be a little faster.

comment:8 Changed 11 years ago by novoselt

Made user's radius more important (new code block in ToricPlotter.__init__), so that it is possible to make it small even if it cuts off some generators.

comment:9 Changed 11 years ago by novoselt

Depends on #9972.

comment:10 Changed 11 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

Nice! Tested against sage-4.6.1alpha3

comment:11 Changed 11 years ago by jdemeyer

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