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 )
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)
Change History (24)
comment:1 Changed 7 years ago by
- Description modified (diff)
Changed 7 years ago by
comment:2 Changed 7 years ago by
- Description modified (diff)
comment:3 Changed 7 years ago by
- Status changed from new to needs_review
comment:4 Changed 7 years ago by
- Description modified (diff)
- Summary changed from polar method of polyhedra is incorrect to Polyhedral bugfixes and improvements
comment:5 Changed 7 years ago by
- Cc vbraun added
comment:6 Changed 7 years ago by
- 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 newfaces(dim)
method that returns the faces in the given dimension. The docstring shows a one-liner to go from there to the oldface_dual_descriptions
method.
comment:7 Changed 7 years ago by
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
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
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
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()
oroffset_polyhedron()
(exploded?),plot(color)
to implement what you want.
Thoughts?
comment:12 Changed 7 years ago by
For the patchbot:
Apply trac_13312_polyhedral_neg_polar.patch
comment:13 Changed 6 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:14 Changed 6 years ago by
- Status changed from needs_review to needs_work
This needs to be rebased on 5.13.beta0.
comment:15 Changed 6 years ago by
- 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
- Description modified (diff)
comment:17 Changed 6 years ago by
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
Probably caused by #14358
comment:19 Changed 6 years ago by
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
- 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
- Resolution set to fixed
- Status changed from positive_review to closed
Fixes the polar method of Polyhedron (was inverted)