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:

Status badges

Description (last modified by chapoton)

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)

trac_13249_fix_lrs_volume.patch (692 bytes) - added by mhampton 9 years ago.
trac_13249.patch (731 bytes) - added by saliola 8 years ago.
trac_13249_volume.patch (4.2 KB) - added by vbraun 8 years ago.
Rebased patch
trac_13249_vb.patch (704 bytes) - added by vbraun 8 years ago.
Rebased patch
trac_13249-review-fc.patch (4.0 KB) - added by chapoton 8 years ago.

Download all attachments as: .zip

Change History (20)

Changed 9 years ago by mhampton

comment:1 Changed 9 years ago by mhampton

  • Status changed from new to needs_review

comment:2 Changed 8 years ago by saliola

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.

Changed 8 years ago by saliola

comment:3 Changed 8 years ago by saliola

Apply: trac_13249.patch

comment:4 Changed 8 years ago by vbraun

Sounds good. Can you include a commit message? Just edit the patch with a text editor...

comment:5 Changed 8 years ago by vbraun

  • Authors changed from Marshall Hampton to Marshall Hampton, Franco Saliola, Volker Braun
  • 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 vbraun

For the patchbot:

apply trac_13249_vb.patch, trac_13249_volume.patch

comment:7 Changed 8 years ago by vbraun

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 vbraun

For the patchbot:

apply trac_13249_vb.patch, trac_13249_volume.patch

Changed 8 years ago by vbraun

Rebased patch

comment:9 Changed 8 years ago by vbraun

Rebased for Sage-5.8.beta4

Now would be a good time to review this rather trivial ticket *hint* *hint*

Changed 8 years ago by vbraun

Rebased patch

comment:10 Changed 8 years ago by vbraun

For the patchbot:

apply trac_13249_vb.patch, trac_13249_volume.patch

comment:11 Changed 8 years ago by vbraun

  • Dependencies set to #14184

Changed 8 years ago by chapoton

comment:12 Changed 8 years ago by chapoton

  • 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 chapoton

  • Reviewers set to Frédéric Chapoton

comment:14 Changed 8 years ago by vbraun

  • Status changed from needs_review to positive_review

Reviewer patch looks good to me.

comment:15 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.9.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.