Opened 3 years ago
Closed 3 years ago
#27673 closed defect (fixed)
Fix Polyhedron.volume() in 0-dimensional space
Reported by: | gh-kliem | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.8 |
Component: | geometry | Keywords: | polytopes, volume |
Cc: | jipilab | Merged in: | |
Authors: | Jonathan Kliem | Reviewers: | Jean-Philippe 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 3 years ago by
- Description modified (diff)
comment:2 Changed 3 years ago by
- Branch set to public/27673
- Commit set to 3bd7a03c38bc78ca938a949fb3483109f3f3978e
comment:3 Changed 3 years ago by
- Status changed from new to needs_review
comment:4 Changed 3 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 follow-up: ↓ 7 Changed 3 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 full-proof 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 0-dimensional 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 3 years ago by
- Cc jipilab added
comment:7 in reply to: ↑ 5 Changed 3 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 well-defined 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 full-proof 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 0-dimensional 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 3 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 3 years ago by
- Description modified (diff)
comment:10 Changed 3 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 3 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 3 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 3 years ago by
- Description modified (diff)
- Summary changed from AttributeError of Polyhedron.volume() to Fix Polyhedron.volume() in 0-dimensional space
comment:14 Changed 3 years ago by
- Commit changed from 7995b24ac3eafa0c3c4404257478b67840a48950 to 98e05b5d3e5681d7321511c5208d7ce9bfc9bea6
comment:15 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:16 Changed 3 years ago by
- Keywords polytopes volume added
- Priority changed from minor to major
- Reviewers set to Jean-Philippe Labbé
- Status changed from needs_review to positive_review
Looks good to me now.
comment:17 follow-up: ↓ 18 Changed 3 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 3 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 0-dimensional 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 3 years ago by
Oh ok, I misread the docstring.
Still a merge conflict though.
comment:20 Changed 3 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 spkg-configure.m4 as it was
|
64f4b4d | update docs, as discussed on #24905
|
2fa1373 | Mention libterm-readkey-perl
|
406ece2 | Mention libterm-readline-gnu-perl
|
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 3 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 0-dimensional volume
|
comment:22 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:23 follow-up: ↓ 24 Changed 3 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 3 years ago by
Replying to gh-kliem:
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 3 years ago by
- Status changed from needs_review to needs_work
comment:26 Changed 3 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 3 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 3 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 0-dimensional volume
|
comment:29 Changed 3 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 3 years ago by
- Status changed from needs_review to positive_review
Looks good to me now.
comment:31 Changed 3 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()