Opened 8 years ago
Last modified 7 years ago
#11763 closed enhancement
Parents for polyhedra — at Version 25
Reported by: | vbraun | Owned by: | mhampton |
---|---|---|---|
Priority: | major | Milestone: | sage-5.6 |
Component: | geometry | Keywords: | |
Cc: | robertwb | Merged in: | |
Authors: | Volker Braun | Reviewers: | |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #11634 | Stopgaps: |
Description (last modified by )
The Polyhedron class is, so far, free-standing with some sort of coercion for the base ring tacked manually. This ticket strives to add parents for polyhedra and make the base ring coercion work more naturally.
There will be 3 supported base rings:
- ZZ (meaning that the polyhedron is a lattice polytope, that is, both H- and V-representation are defined over ZZ)
- RDF
Apply:
Change History (32)
comment:1 Changed 8 years ago by
comment:2 Changed 8 years ago by
We don't have an implementation of the double description algorithm for other base rings, so we wouldn't be able to compute anything.
comment:3 Changed 8 years ago by
- Description modified (diff)
- Status changed from new to needs_review
This is now ready for inclusion. Marshall, are you interested in reviewing this patch and its dependency? ;-)
comment:4 Changed 8 years ago by
- Description modified (diff)
comment:5 Changed 8 years ago by
Fix comparison of H/V-representation objects:
sage: triangle = Polyhedron([(0,0), (1,0), (0,1)]) sage: ieq = triangle.inequality_generator().next() sage: ieq == copy(ieq) False
Now returns True
, as it should.
comment:6 Changed 8 years ago by
- Description modified (diff)
The new patch I posted at #11634 broke these, so I rebased them.
Apply trac_11763_ZZ_polyhedron-rebase.patch, trac_11763_parents-rebase.patch
comment:7 Changed 8 years ago by
Apply trac_11763_zz_polyhedron-rebase.patch, trac_11763_parents-rebase.patch
(bloody patchbot!)
comment:8 Changed 8 years ago by
No matter what I do, the patchbot won't pick up the two patches above, so I've done a single qfolded patch. Hopefully this will now work!
comment:9 Changed 8 years ago by
- Description modified (diff)
comment:10 Changed 8 years ago by
Sorry, I made a hash of rebasing the patch, so here's a new version.
comment:11 Changed 8 years ago by
This changes the behavior of the .vertices() method, returning "sage.geometry.polyhedron.representation.Vertex" objects instead of simple lists of coordinates. This breaks some code of mine, so I think there should be a deprecation warning before introducing this. I don't really like the behavior, but I see that .vertices_list() does return a list of lists of coordinates.
This patch also seems to slow things down a bit, but I haven't carefully tested that aspect.
comment:12 Changed 8 years ago by
The Vertex
object implements the list interface, so any code that doesn't just test isinstance(-,list)
should work fine. Plus, you can't really deprecate something unless you want to remove it later. I added the 'vertices_list` method so people can keep their broken code with minimal patching if they want.
comment:13 Changed 8 years ago by
Well, no, there is quite a bit of my code that does not work fine, and I am not talking about artificial test cases. Here is a shortened example of something this patch breaks:
c = polytopes.n_cube(3) v = c.vertices()[0] point(v)
This now gives a "TypeError?: 'sage.rings.integer.Integer' object does not support indexing".
comment:14 Changed 8 years ago by
For the record: #12541 and #12544 have a similar situation. I replaced tuples with tuple-like containers and some stuff got broken. As it turned out, it was not too difficult to fix it by eliminating some "hard typechecks", although they are still waiting for an approval. If this is the case here, perhaps it is also not too difficult to fix.
As I understand it, direct typechecking is not welcomed in either Python or Sage, so if the new class implements list interface, it should be OK to use in place of the old one. If it does break some code, it is a bug and it should be fixed before merging, but not a reason for avoiding the change entirely or inventing elaborate deprecation scheme...
comment:15 Changed 8 years ago by
- Status changed from needs_review to needs_work
- Work issues set to doctest failure
There is a doctest failing on 5.0.beta10, according to the patchbot (something to do with the new associahedron class from #10817)
comment:16 Changed 8 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
The associahedron needed to be translated to the parent/element framework as well. I've fixed this. Also, I rediffed trac_11763-polyhedra_coercion_folded.2.patch because it only applied with some fuzz on sage-5.0.beta11
comment:17 Changed 8 years ago by
- Description modified (diff)
The final patch fixes the point()
command to not make dumb type checks.
comment:18 Changed 8 years ago by
I noted a problem in the documentatation build, the updated patch fixes just that.
comment:19 Changed 8 years ago by
Apply trac_11763-polyhedra_coercion_folded.2.patch, trac_11763_fix_associahedron.patch, trac_11763_relax_typechecks.patch
(for patchbot)
comment:20 Changed 8 years ago by
I thought the point of having "Apply" section in the description was to make the patchbot happy, why does it need to be duplicated?..
comment:21 Changed 8 years ago by
It was previously applying the patches in an order other than that listed on the ticket description (with coercion_folded.2.patch last rather than first). I didn't think to check whether the patches were applying happily anyway; as it happens they were. Sorry for the noise!
comment:22 Changed 8 years ago by
Oh, I don't complain about your post - I complain about patchbot not figuring out what is the correct order despite a clear description ;-)
comment:23 Changed 8 years ago by
- Status changed from needs_review to needs_work
- Work issues changed from doctest failure to remove trailing whitespaces
comment:24 Changed 8 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
- Work issues remove trailing whitespaces deleted
We don't have a policy on trailing white space, so its completely useless bikeshedding to require it one way or another. But since this ticket is the special case where we create a new subdirectory layout, I used a simple sed script to remove trailing whitespace.
comment:25 Changed 8 years ago by
- Description modified (diff)
Would there be any benefit in supporting real fields of arbitrary precision?