#11763 closed enhancement (fixed)
Parents for polyhedra
Reported by: | vbraun | Owned by: | mhampton |
---|---|---|---|
Priority: | major | Milestone: | sage-5.6 |
Component: | geometry | Keywords: | |
Cc: | robertwb | Merged in: | sage-5.6.beta1 |
Authors: | Volker Braun | Reviewers: | Dmitrii Pasechnik |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #11634, #13109, #11310 | 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:
Attachments (13)
Change History (75)
comment:1 Changed 11 years ago by
comment:2 Changed 11 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 11 years ago by
Authors: | → Volker Braun |
---|---|
Description: | modified (diff) |
Status: | new → needs_review |
This is now ready for inclusion. Marshall, are you interested in reviewing this patch and its dependency? ;-)
comment:4 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:5 Changed 11 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.
Changed 11 years ago by
Attachment: | trac_11763_ZZ_polyhedron-rebase.patch added |
---|
Rebased for new patch at 11634
Changed 11 years ago by
Attachment: | trac_11763_parents-rebase.patch added |
---|
Rebased for new patch at #11634
comment:6 Changed 11 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 11 years ago by
Apply trac_11763_zz_polyhedron-rebase.patch, trac_11763_parents-rebase.patch
(bloody patchbot!)
comment:8 Changed 11 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 11 years ago by
Description: | modified (diff) |
---|
Changed 11 years ago by
Attachment: | trac_11763-polyhedra_coercion_folded.patch added |
---|
Apply only this patch. Patch against 5.0.beta7
comment:10 Changed 11 years ago by
Sorry, I made a hash of rebasing the patch, so here's a new version.
comment:11 Changed 11 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 11 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 11 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 11 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 11 years ago by
Status: | needs_review → needs_work |
---|---|
Work issues: | → 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 11 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_work → 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 11 years ago by
Description: | modified (diff) |
---|
The final patch fixes the point()
command to not make dumb type checks.
comment:18 Changed 11 years ago by
I noted a problem in the documentatation build, the updated patch fixes just that.
comment:19 Changed 11 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 11 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 11 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 11 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 11 years ago by
Status: | needs_review → needs_work |
---|---|
Work issues: | doctest failure → remove trailing whitespaces |
comment:24 Changed 11 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_work → needs_review |
Work issues: | remove trailing whitespaces |
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 11 years ago by
Description: | modified (diff) |
---|
comment:26 Changed 11 years ago by
Status: | needs_review → needs_work |
---|
- There is now an error to be corrected, see bot report for 5.0rc0. This comes from a strange comparison with 'anything', which looks weird.
- Maybe fold all the patches into one ?
- In the part about associahedra, you replaced the sentence "Returns the Cartan type of self." by the sentence "Returns the Cartan type". This seems to be less informative.
Changed 11 years ago by
Attachment: | trac_11763-polyhedra_coercion_folded.2.2.patch added |
---|
Updated patch
comment:27 Changed 11 years ago by
I've changed the cmp()
doctest to use abs(cmp())
to avoid dependence on arbitrary memory positions.
Folding patches is considered harmful since it destroys history without any benefit.
A method always returns information about self
, you could add "... of self" to every docstring.
comment:28 Changed 11 years ago by
Status: | needs_work → needs_review |
---|
comment:29 Changed 11 years ago by
Rediffed for sage-5.0.beta3 because #12340 introduced a newline in one of the docstrings. No actual code changed.
comment:30 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:31 Changed 11 years ago by
Description: | modified (diff) |
---|
Apply trac_11763-polyhedra_coercion_folded.2.patch, trac_11763_fix_associahedron.patch, trac_11763_relax_typechecks.patch, trac_11763_remove_trailing_whitespace.patch
comment:32 Changed 11 years ago by
Dependencies: | #11634 → #11634, #13109 |
---|
comment:33 Changed 11 years ago by
Dependencies: | #11634, #13109 → #11634, #13109, #11310 |
---|
#11310 fixes all the catch-all except:
clauses, breaking this patch.
comment:34 Changed 11 years ago by
I'm getting one error when testing:
File "/home/novoselt/sage-5.2.rc1/devel/sage-main/sage/libs/ppl.pyx", line 119: sage: Polyhedron(vertices=gs, backend='cddr') # long time (3s on sage.math, 2011) Exception raised: Traceback (most recent call last): File "/home/novoselt/sage-5.2.rc1/local/bin/ncadoctest.py", line 1231, in run_one_test self.run_one_example(test, example, filename, compileflags) File "/home/novoselt/sage-5.2.rc1/local/bin/sagedoctest.py", line 38, in run_one_example OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags) File "/home/novoselt/sage-5.2.rc1/local/bin/ncadoctest.py", line 1172, in run_one_example compileflags, 1) in test.globs File "<doctest __main__.example_0[37]>", line 1, in <module> Polyhedron(vertices=gs, backend='cddr') # long time (3s on sage.math, 2011)###line 119: sage: Polyhedron(vertices=gs, backend='cddr') # long time (3s on sage.math, 2011) File "/home/novoselt/sage-5.2.rc1/local/lib/python/site-packages/sage/misc/decorators.py", line 691, in wrapper return func(*args, **kwds) File "/home/novoselt/sage-5.2.rc1/local/lib/python/site-packages/sage/geometry/polyhedron/constructor.py", line 325, in Polyhedron parent = Polyhedra(base_ring, ambient_dim, backend=backend) File "/home/novoselt/sage-5.2.rc1/local/lib/python/site-packages/sage/geometry/polyhedron/parent.py", line 56, in Polyhedra ') implemented for given basering (='+str(base_ring)+').') ValueError: No such backend (=cddr) implemented for given basering (=Integer Ring).
comment:35 Changed 11 years ago by
I fixed the doctest (its backend=cdd
not cddr
) and added documentation to the sage.geometry.polyhedron.parent.Polyhedra
constructor function.
comment:36 Changed 11 years ago by
Updated patch fixes one important issue. The inherited comparison for PolyhedronFace_base
was too lax, which resulted in identical (and not just isomorphic) face lattices in some cases. Fixed now.
comment:37 Changed 10 years ago by
Dependencies: | #11634, #13109, #11310 → #11634, #13109, #11310, #13638 |
---|
Rebased for #13638
Changed 10 years ago by
Attachment: | trac_11763_remove_trailing_whitespace.patch added |
---|
Updated patch
comment:38 Changed 10 years ago by
Description: | modified (diff) |
---|
I've changed the PolyhedronFace
class to just derive from SageObject
this should be clearer. Also, the more trivial patches are folded into the first.
It would be great if this ticket could be reviewed before the heat death of the universe.
comment:40 Changed 10 years ago by
Apply only trac_11763-polyhedra_coercion_folded.2.patch trac_11763_fix_associahedron.patch
comment:41 Changed 10 years ago by
Ok, let us forget about trailing whitespaces.
But what about the startup plugin which is complaining here ?
comment:42 Changed 10 years ago by
I agree that using lazy imports would be good. I'll give it a whirl.
comment:43 Changed 10 years ago by
Description: | modified (diff) |
---|
Patch loads Polyhedron lazily, so we don't import anything on startup.
comment:45 Changed 10 years ago by
Apply trac_11763-polyhedra_coercion_folded.2.patch trac_11763_fix_associahedron.patch trac_11763_lazy_import.patch
comment:46 Changed 10 years ago by
Dependencies: | #11634, #13109, #11310, #13638 → #11634, #13109, #11310 |
---|
I switched the order this patch and #13638.
comment:47 Changed 10 years ago by
Status: | needs_review → positive_review |
---|
comment:48 follow-up: 49 Changed 10 years ago by
The patchbot seems to follow the patch order in the Attachments list, rather than in the ticket description Apply list.
comment:49 Changed 10 years ago by
Cc: | robertwb added |
---|
Replying to dimpase:
The patchbot seems to follow the patch order in the Attachments list, rather than in the ticket description Apply list.
If I am reading the right patchbot source, it seems that it looks for apply
in the whole rss feed...
comment:50 Changed 10 years ago by
For the patchbot:
Apply trac_11763-polyhedra_coercion_folded.2.patch, trac_11763_fix_associahedron.patch, trac_11763_lazy_import.patch
comment:51 Changed 10 years ago by
Reviewers: | → Dmitrii Pasechnik |
---|
comment:52 Changed 10 years ago by
On some systems, such as OS X >= 10.6 and Itanium:
sage -t --long -force_lib devel/sage/sage/geometry/polyhedron/base.py ********************************************************************** File "/Users/dehayebuildbot/build/sage/dehaye/dehaye_full/build/sage-5.6.beta0/devel/sage-main/sage/geometry/polyhedron/base.py", line 385: sage: cmp(P, 'string') Expected: -1 Got: 1 **********************************************************************
comment:53 Changed 10 years ago by
Status: | positive_review → needs_work |
---|
Changed 10 years ago by
Attachment: | trac_11763-polyhedra_coercion_folded.2.patch added |
---|
Updated patch
comment:56 Changed 10 years ago by
Merged in: | → sage-5.6.beta1 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Changed 10 years ago by
Attachment: | trac_11763-polyhedra_coercion_folded.3.patch added |
---|
comment:58 Changed 10 years ago by
Hey Volker,
Do you remember what was the reason for this change in normalize_rays
?
-
sage/geometry/cone.py
diff --git a/sage/geometry/cone.py b/sage/geometry/cone.py
a b 568 568 V = lattice.base_extend(QQ) 569 569 for n, ray in enumerate(rays): 570 570 try: 571 ray = V(ray) 571 if isinstance(ray, (list, tuple, V._element_class)): 572 ray = V(ray) 573 else: 574 ray = V(list(ray)) 572 575 except TypeError: 573 576 raise TypeError("cannot convert %s to %s!" % (ray, V)) 574 577 if ray.is_zero():
It seems that if V
does not want to take a ray
, we should not force it.
comment:59 Changed 10 years ago by
I think the idea is to allow arbitrary iterables in normalize_rays
. Why not? Its an auxiliary function, and the more general it is the better.
comment:60 follow-up: 61 Changed 10 years ago by
I just was wondering what was the particular use case - found this bit because of the fuzz with another patch. I don't mind it, although changing what V.__call__
accepts seems a more pleasant solution ;-)
comment:61 follow-up: 62 Changed 10 years ago by
Replying to novoselt:
I don't mind it, although changing what
V.__call__
accepts seems a more pleasant solution ;-)
Wasn't that one of our design decisions, that the ToricLattice should forbid conversion between the lattice and its dual?
comment:62 Changed 10 years ago by
Replying to vbraun:
Wasn't that one of our design decisions, that the ToricLattice should forbid conversion between the lattice and its dual?
We prohibit coercion, conversion is certainly possible, and here V
is the spanned vector space anyway. But I've also hit many times a situation when a standard constructor in Sage would not take generators as input, which is probably what has happened here.
Would there be any benefit in supporting real fields of arbitrary precision?