Opened 7 years ago

Closed 6 years ago

#13312 closed defect (fixed)

Polyhedral bugfixes and improvements

Reported by: mhampton Owned by: mhampton
Priority: major Milestone: sage-6.1
Component: geometry Keywords: polyhedra, polar
Cc: vbraun Merged in:
Authors: Marshall Hampton, Volker Braun Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: u/vbraun/polyhedral_neg_polar (Commits) Commit: 9e2fe957e16c53fe39df2d58cb739b0dc416bfd6
Dependencies: #11763 Stopgaps:

Description (last modified by vbraun)

The .polar() method of the Polyhedron class is incorrect, currently it returns the inversion of the correct answer. Thanks to sarah-marie belcastro for pointing this out.

Another bugfix/improvement is adding support for negation, which inverts polyhedra. Implementing this revealed that scalar multiplication by negative numbers gave incorrect answers for polyhedra with rays.

Finally this adds a convenience function for obtained faces in terms of vertex and inequality indices.

Attachments (3)

trac_13312_fix_polar_method_polyhedron.patch (1.4 KB) - added by mhampton 7 years ago.
Fixes the polar method of Polyhedron (was inverted)
trac_13312_polyhedral_bugfixes_and_improvements.patch (4.8 KB) - added by mhampton 7 years ago.
Fixes polar and scalar multiplication bugs
trac_13312_polyhedral_neg_polar.patch (6.6 KB) - added by vbraun 7 years ago.
Updated patch

Download all attachments as: .zip

Change History (24)

comment:1 Changed 7 years ago by mhampton

  • Description modified (diff)

Changed 7 years ago by mhampton

Fixes the polar method of Polyhedron (was inverted)

comment:2 Changed 7 years ago by mhampton

  • Description modified (diff)

comment:3 Changed 7 years ago by mhampton

  • Status changed from new to needs_review

comment:4 Changed 7 years ago by mhampton

  • Description modified (diff)
  • Summary changed from polar method of polyhedra is incorrect to Polyhedral bugfixes and improvements

Changed 7 years ago by mhampton

Fixes polar and scalar multiplication bugs

comment:5 Changed 7 years ago by mhampton

  • Cc vbraun added

comment:6 Changed 7 years ago by vbraun

  • Authors changed from Marshall Hampton to Marshall Hampton, Volker Braun
  • Dependencies set to #11763
  • Description modified (diff)
  • rebased on top of #11763
  • added commit message
  • I'm against the face_dual_descriptions method. Never return lists (mutable), and never return integer indices into internal data structures if you can help it. Just return the actual object, its the same to Python (where objects are really references to objects anyways). But I understand that it would be desirable to access the faces without having to dig through the face lattice. I've replaced it with a new faces(dim) method that returns the faces in the given dimension. The docstring shows a one-liner to go from there to the old face_dual_descriptions method.

comment:7 Changed 7 years ago by mhampton

I'd really like to have the functionality of the face_dual_descriptions method. I disagree that your docstring is really a one-liner, and I don't think that would be very easy for Sage/Python? newbies to figure out. I have talked to a couple of people who would like a platform for polyhedral computation who have tried Sage and find it very difficult to accomplish what they would like to do. So I think perhaps we could add your faces(dim) method and an improved version of the face_dual_descriptions method which could return a tuple instead of a list if that makes you happier.

comment:8 Changed 7 years ago by vbraun

What do you and/or your colleagues actually want to do? I doubt that anybody wants tuples of tuples of tuples of nondescript integers thrown at them. This is both non-discoverable (no way to figure out what the integers mean) while at the same time very error-prone (accidentally use the index in the wrong list and you'll get hard-to-find bugs that yield the wrong result). Its arguably true that many students were harmed with such anti-patterns, but its never too late to to change.

Right now the PolyhedronFace class isn't really complete, so presumably we need to flesh it more out so that you can get everything you need from the face.

comment:9 Changed 7 years ago by mhampton

OK, I am working on some sort of compromise patch and I noticed that your patch (trac_13312_polyhedral_neg_polar.patch) doesn't apply - it looks like it applies to some intermediate version you had - ?

I actually find the output of the function I wrote convenient of a number of purposes. Here's a couple of examples of things I have wanted to do before (I can't really speak for others, I just know they find the present capabilities lacking): Take all the facets of a polyhedron, and move them away from the center of the polyhedron (sort of explode it). If the polyhedron is 3-dimensional, show these facets in different colors. Or another example: color all of the facets of a 3-d polyhedron whose outward normals satisfy some condition.

comment:10 Changed 7 years ago by vbraun

Patch applies to sage-5.3.beta1, I'm building sage-5.3.rc0 now to see if there is any issue.

I thought about how to handle things and I'd like to

  • Not make PolyhedronFace derive from polyhedron - can be confusing as not every Polyhedron method makes sense.
  • The PolyhedronFace should have a polyhedron() method to get it as a polyhedron without reference to ambient
  • Other methods outward_normal() or offset_polyhedron() (exploded?), plot(color) to implement what you want.

Thoughts?

Changed 7 years ago by vbraun

Updated patch

comment:11 Changed 7 years ago by vbraun

  • Description modified (diff)

Rebased for #11763

comment:12 Changed 7 years ago by vbraun

For the patchbot:

Apply trac_13312_polyhedral_neg_polar.patch

comment:13 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:14 Changed 6 years ago by chapoton

  • Status changed from needs_review to needs_work

This needs to be rebased on 5.13.beta0.

comment:15 Changed 6 years ago by vbraun

  • Branch set to u/vbraun/polyhedral_neg_polar
  • Commit set to 9e2fe957e16c53fe39df2d58cb739b0dc416bfd6
  • Status changed from needs_work to needs_review

Rebased


New commits:

[changeset:9e2fe95]face lattice posit now uses facade, so there is no element attribute any more
[changeset:549683a]Allow unary negation and flip sign in the polar() method

comment:16 Changed 6 years ago by vbraun

  • Description modified (diff)

comment:17 Changed 6 years ago by ncohen

Yooooooooooooo !!

I agree with this patch, though I don't understand one bit : why this change in .polar ?

-  verts = [list(v.vector() - self.center()) for v in self.vertex_generator()]
+  verts = [list(self.center() - v.vector()) for v in self.vertex_generator()]

If it is just to have your example of code work, shouldn't you change the constructor instead ?

By the way, running polytopes.n_cube(3).show() just gives me a black Jmol window. I mean, empty. It's like the world is missing a cube.

Nathann

comment:18 Changed 6 years ago by vbraun

Probably caused by #14358

comment:19 Changed 6 years ago by vbraun

The sign change in polar() is what this ticket is about. Whats your question, and why should we change the unrelated ctor?

comment:20 Changed 6 years ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to positive_review

Oh sorry. I thought that you just changed the result of this .polar() thing so that the two constructors would be equal, and not just isomorphic. I was saying that instead of changing the behaviour of this function to get this result, you should just multiply one of the two constructors by -1 which would appear more correct :-P

Okayokay, then if this def of polar is the good one, this patch is good to go as all other additions seem good too ;-)

Nathann

comment:21 Changed 6 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.