I think its time to start a tradition where I'm asking for ticket reviews ;-)
$ git trac get 13312 ============================================================================== Trac #13312: Polyhedral bugfixes and improvements 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. Status: needs_review Component: geometry Last modified: 2013-12-23 15:10:58 Created: 2012-07-31 13:34:10 UTC Report upstream: N/A Authors: Marshall Hampton, Volker Braun Reviewers: Branch: u/vbraun/polyhedral_neg_polar Keywords: polyhedra, polar Dependencies: #11763 ------------------------------------------------------------------------------ Comment #1 by mhampton at 2012-08-03 15:50:34 UTC: [Description] modified ------------------------------------------------------------------------------ Comment # by mhampton at 2012-08-03 15:51:17 UTC: Fixes the polar method of Polyhedron (was inverted) [Attachment] "trac_13312_fix_polar_method_polyhedron.patch" added ------------------------------------------------------------------------------ Comment #2 by mhampton at 2012-08-03 15:54:03 UTC: [Description] modified ------------------------------------------------------------------------------ Comment #3 by mhampton at 2012-08-03 15:54:22 UTC: [Status] changed from new to needs_review ------------------------------------------------------------------------------ Comment #4 by mhampton at 2012-08-04 16:27:56 UTC: [Description] modified [Summary] changed to Polyhedral bugfixes and improvements ------------------------------------------------------------------------------ Comment # by mhampton at 2012-08-04 16:28:27 UTC: Fixes polar and scalar multiplication bugs [Attachment] "trac_13312_polyhedral_bugfixes_and_improvements.patch" added ------------------------------------------------------------------------------ Comment #5 by mhampton at 2012-08-04 16:29:31 UTC: [Cc] set to vbraun ------------------------------------------------------------------------------ Comment #6 by vbraun at 2012-08-04 22:59:17 UTC: * 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. [Authors] changed from Marshall Hampton to Marshall Hampton, Volker Braun [Dependencies] set to #11763 [Description] modified ------------------------------------------------------------------------------ Comment #7 by mhampton at 2012-08-07 18:46:56 UTC: 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 by vbraun at 2012-08-08 01:05:03 UTC: 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 by mhampton at 2012-08-26 22:34:01 UTC: 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 by vbraun at 2012-08-27 03:01:53 UTC: 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? ------------------------------------------------------------------------------ Comment # by vbraun at 2012-10-23 21:48:03 UTC: Updated patch [Attachment] "trac_13312_polyhedral_neg_polar.patch" added ------------------------------------------------------------------------------ Comment #11 by vbraun at 2012-10-23 21:49:11 UTC: Rebased for #11763 [Description] modified ------------------------------------------------------------------------------ Comment #12 by vbraun at 2012-11-06 21:11:38 UTC: For the patchbot: Apply trac_13312_polyhedral_neg_polar.patch ------------------------------------------------------------------------------ Comment #13 by jdemeyer at 2013-08-13 15:35:53 UTC: [Milestone] changed from sage-5.11 to sage-5.12 ------------------------------------------------------------------------------ Comment #14 by chapoton at 2013-10-19 20:12:18 UTC: This needs to be rebased on 5.13.beta0. [Status] changed from needs_review to needs_work ------------------------------------------------------------------------------ Comment #15 by vbraun at 2013-10-19 20:54:25 UTC: 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|| [Branch] set to u/vbraun/polyhedral_neg_polar [Commit] set to 9e2fe957e16c53fe39df2d58cb739b0dc416bfd6 [Status] changed from needs_work to needs_review ------------------------------------------------------------------------------ Comment #16 by vbraun at 2013-12-23 15:10:58 UTC: [Description] modified ------------------------------------------------------------------------------ URL: http://trac.sagemath.org/13312 ============================================================================== -- You received this message because you are subscribed to the Google Groups "sage-devel" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/sage-devel. For more options, visit https://groups.google.com/groups/opt_out.
