Opened 2 years ago
Closed 2 years ago
#27673 closed defect (fixed)
Fix Polyhedron.volume() in 0dimensional space
Reported by:  ghkliem  Owned by:  

Priority:  major  Milestone:  sage8.8 
Component:  geometry  Keywords:  polytopes, volume 
Cc:  jipilab  Merged in:  
Authors:  Jonathan Kliem  Reviewers:  JeanPhilippe Labbé 
Report Upstream:  N/A  Work issues:  
Branch:  a7bf147 (Commits, GitHub, GitLab)  Commit:  a7bf147ff3f043167c420e2525ada31068417489 
Dependencies:  Stopgaps: 
Description (last modified by )
In dimension 0, we now use the counting measure for volume:
sage: Polyhedron(vertices=[[]]).volume() 1 sage: Polyhedron(vertices=[]).volume() 0
Originally there was an attribute error:
sage: Polyhedron(vertices=[[]]).volume() Traceback (most recent call last) ... AttributeError: 'int' object has no attribute 'set_immutable'
Change History (31)
comment:1 Changed 2 years ago by
 Description modified (diff)
comment:2 Changed 2 years ago by
 Branch set to public/27673
 Commit set to 3bd7a03c38bc78ca938a949fb3483109f3f3978e
comment:3 Changed 2 years ago by
 Status changed from new to needs_review
comment:4 Changed 2 years ago by
 Description modified (diff)
When thinking about it again, I'm not sure anymore that this is the correct output.
However, an AttributeError? is strange and the dostring does not indicate, what should happen.
Also it's confusing that the empty Polyhedron gives an output.
comment:5 followup: ↓ 7 Changed 2 years ago by
This might be related to #17339. If you want to have fun in the maze of defaulting behaviour, I challenge you to come up with a fullproof default behavior that will please everyone.
One thing that you should be aware of, is the current handling of things via different H and V inputs.
sage: P = Polyhedron(vertices=[]) sage: Q = Polyhedron(vertices=[[]]) sage: P The empty polyhedron in ZZ^0 sage: Q A 0dimensional polyhedron in ZZ^0 defined as the convex hull of 1 vertex
The first one is the empty polyhedron. The second one is just one point, i.e. the full space. They are not the same.
comment:6 Changed 2 years ago by
 Cc jipilab added
comment:7 in reply to: ↑ 5 Changed 2 years ago by
Well, I can live with a meaningful 'NotImplementedError?'.
Either there is a measure with ambient dimension zero or not. So the empty and the one point polyhedron in zero dimensional space should either have a welldefined behaiviour or not.
The reason I looked into it is that the original error is hard to trace back. It took me a bit to figure out, what went wrong there.
Replying to jipilab:
This might be related to #17339. If you want to have fun in the maze of defaulting behaviour, I challenge you to come up with a fullproof default behavior that will please everyone.
One thing that you should be aware of, is the current handling of things via different H and V inputs.
sage: P = Polyhedron(vertices=[]) sage: Q = Polyhedron(vertices=[[]]) sage: P The empty polyhedron in ZZ^0 sage: Q A 0dimensional polyhedron in ZZ^0 defined as the convex hull of 1 vertexThe first one is the empty polyhedron. The second one is just one point, i.e. the full space. They are not the same.
comment:8 Changed 2 years ago by
 Commit changed from 3bd7a03c38bc78ca938a949fb3483109f3f3978e to cfa12c74b0387fe8d400454f0c33a691e9d08058
Branch pushed to git repo; I updated commit sha1. New commits:
cfa12c7  volume in dimension 0 is counting measure

comment:9 Changed 2 years ago by
 Description modified (diff)
comment:10 Changed 2 years ago by
 Commit changed from cfa12c74b0387fe8d400454f0c33a691e9d08058 to 7995b24ac3eafa0c3c4404257478b67840a48950
Branch pushed to git repo; I updated commit sha1. New commits:
7995b24  a bit nicer documentation

comment:11 Changed 2 years ago by
 Status changed from needs_review to needs_work
You should change the title and description of the ticket to reflect faithfully the changes inside the ticket.
comment:12 Changed 2 years ago by
Failed doctest:
sage t long src/sage/geometry/polyhedron/base.py ********************************************************************** File "src/sage/geometry/polyhedron/base.py", line 5251, in sage.geometry.polyhedron.base.Polyhedron_base.volume Failed example: P = Polyhedron(vertices=[]); P Expected: The empty polyhedron in ZZ^0 0 Got: The empty polyhedron in ZZ^0 **********************************************************************
comment:13 Changed 2 years ago by
 Description modified (diff)
 Summary changed from AttributeError of Polyhedron.volume() to Fix Polyhedron.volume() in 0dimensional space
comment:14 Changed 2 years ago by
 Commit changed from 7995b24ac3eafa0c3c4404257478b67840a48950 to 98e05b5d3e5681d7321511c5208d7ce9bfc9bea6
comment:15 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:16 Changed 2 years ago by
 Keywords polytopes volume added
 Priority changed from minor to major
 Reviewers set to JeanPhilippe Labbé
 Status changed from needs_review to positive_review
Looks good to me now.
comment:17 followup: ↓ 18 Changed 2 years ago by
 Status changed from positive_review to needs_work
Merge conflict, but isn't it also also mathematically wrong:
 In the "ambient" measure, if ambient dim > 0, the volume of a point is 0
 In the "ambient" measure, if ambient dim == 0, the volume of a point is 1
 In the induced measures, the volume of a point is 1
 The volume of the empty polyhedron is always 0
comment:18 in reply to: ↑ 17 Changed 2 years ago by
Replying to vbraun:
Merge conflict, but isn't it also also mathematically wrong:
 In the "ambient" measure, if ambient dim > 0, the volume of a point is 0
See line above. Which returns 0 in that case.
 In the "ambient" measure, if ambient dim == 0, the volume of a point is 1
 In the induced measures, the volume of a point is 1
A point is projected to 0dimensional space an then 1 is returned.
 The volume of the empty polyhedron is always 0
It has dimension 1 but ambient dimension at least 0, so zero will be returned.
I don't see which criteria is not met.
comment:19 Changed 2 years ago by
Oh ok, I misread the docstring.
Still a merge conflict though.
comment:20 Changed 2 years ago by
 Commit changed from 98e05b5d3e5681d7321511c5208d7ce9bfc9bea6 to 343130cd4a9c32c936e580f3c0e894dae84fb5fa
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
b543c6d  remove an obsolete note in schemes, pep cleanup of zariski_vankampen

12373c6  Change format of a 'not tested' doc test annotation

8783bec  leave GMP's spkgconfigure.m4 as it was

64f4b4d  update docs, as discussed on #24905

2fa1373  Mention libtermreadkeyperl

406ece2  Mention libtermreadlinegnuperl

a031f09  polymake: Mention MOngoDB for polydb

473178e  polymake: Update info on macOS

9a4bac1  polymake: update MongoDB information for linux distros

343130c  Updated SageMath version to 8.8.beta5

comment:21 Changed 2 years ago by
 Commit changed from 343130cd4a9c32c936e580f3c0e894dae84fb5fa to 33b1ff9fd090b5a9272ccf63eacf7f0451e879a8
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
33b1ff9  fixed 0dimensional volume

comment:22 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:23 followup: ↓ 24 Changed 2 years ago by
Why does the merge fail again?
Last time I fixed it, was appearently a waste of time. I guess I could start a new branch again, but only if someone sets it to positive review before the merge fails again.
comment:24 in reply to: ↑ 23 Changed 2 years ago by
Replying to ghkliem:
Why does the merge fail again?
Last time I fixed it, was appearently a waste of time. I guess I could start a new branch again, but only if someone sets it to positive review before the merge fails again.
#25091 Modified the volume function and got merged in 8.8.beta6 that's most likely the conflict.
comment:25 Changed 2 years ago by
 Status changed from needs_review to needs_work
comment:26 Changed 2 years ago by
 Commit changed from 33b1ff9fd090b5a9272ccf63eacf7f0451e879a8 to 7a7e57d005962081f1811fcddd74f11278169b65
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7a7e57d  volume in dimension 0 is counting measure

comment:27 Changed 2 years ago by
 Commit changed from 7a7e57d005962081f1811fcddd74f11278169b65 to df1f5495b83d0c2ae2cd37478f7138041a5508e9
Branch pushed to git repo; I updated commit sha1. New commits:
df1f549  should be back to the original

comment:28 Changed 2 years ago by
 Commit changed from df1f5495b83d0c2ae2cd37478f7138041a5508e9 to a7bf147ff3f043167c420e2525ada31068417489
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a7bf147  fixed 0dimensional volume

comment:29 Changed 2 years ago by
 Status changed from needs_work to needs_review
I have no idea, how one is supposed to get rebase to work.
This is basically just writing the old text in the newest develop again.
comment:30 Changed 2 years ago by
 Status changed from needs_review to positive_review
Looks good to me now.
comment:31 Changed 2 years ago by
 Branch changed from public/27673 to a7bf147ff3f043167c420e2525ada31068417489
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
fixed AttributeError of Polyhedron.volume()