Opened 9 years ago
Closed 8 years ago
#13249 closed defect (fixed)
Volume computation of polyhedra
Reported by: | mhampton | Owned by: | mhampton |
---|---|---|---|
Priority: | major | Milestone: | sage-5.9 |
Component: | geometry | Keywords: | polyhedra, lrs_volume, geometry |
Cc: | vbraun | Merged in: | sage-5.9.beta1 |
Authors: | Marshall Hampton, Franco Saliola, Volker Braun | Reviewers: | Frédéric Chapoton |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #14184 | Stopgaps: |
Description (last modified by )
The recent changes to the geometry/polyhedra module broke support for the lrs_volume() method of the Polyhedron class. This is not tested usually because it relies on the lrs optional package. The fix is very simple, just a couple of imports needed to be added.
While we are at it, implement the native volume computation whose bits and pieces (triangulation and volume of simplices) have been around for a while but haven't been hooked up.
Apply:
Attachments (5)
Change History (20)
Changed 9 years ago by
comment:1 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 Changed 8 years ago by
Changed 8 years ago by
comment:3 Changed 8 years ago by
Apply: trac_13249.patch
comment:4 Changed 8 years ago by
Sounds good. Can you include a commit message? Just edit the patch with a text editor...
comment:5 Changed 8 years ago by
- Description modified (diff)
- Summary changed from lrs_volume is broken by polyedral refactoring to Volume computation of polyhedra
Apologies for taking over the ticket, but this belongs together anyways.
comment:6 Changed 8 years ago by
For the patchbot:
apply trac_13249_vb.patch, trac_13249_volume.patch
comment:7 Changed 8 years ago by
I've added the commit message to Frank's patch. I think thats the version to go with since its less global imports. I'm giving positive review to his patch. If one of you guys could review mine then we can ship it...
comment:8 Changed 8 years ago by
For the patchbot:
apply trac_13249_vb.patch, trac_13249_volume.patch
comment:9 Changed 8 years ago by
Rebased for Sage-5.8.beta4
Now would be a good time to review this rather trivial ticket *hint* *hint*
comment:10 Changed 8 years ago by
For the patchbot:
apply trac_13249_vb.patch, trac_13249_volume.patch
comment:11 Changed 8 years ago by
- Dependencies set to #14184
Changed 8 years ago by
comment:12 Changed 8 years ago by
- Description modified (diff)
I have made a small review patch, including a clean up of the unused or missing import statements (found using pyflakes)
If you are happy with my review patch, and when the bot turns green again, you can set a positive review
for the bot:
Apply trac_13249_vb.patch trac_13249_volume.patch trac_13249-review-fc.patch
comment:13 Changed 8 years ago by
- Reviewers set to Frédéric Chapoton
comment:14 Changed 8 years ago by
- Status changed from needs_review to positive_review
Reviewer patch looks good to me.
comment:15 Changed 8 years ago by
- Merged in set to sage-5.9.beta1
- Resolution set to fixed
- Status changed from positive_review to closed
We came across this exact issue today when exploring the polyhedra functionality in Sage at Sage Days 45. And we came up with the same fix, except that we put the import statements directly in the
lrs_volume
method.