#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) Commit: a7bf147ff3f043167c420e2525ada31068417489
Dependencies: Stopgaps:

Description (last modified by gh-kliem)

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 14 months ago by gh-kliem

  • Description modified (diff)

comment:2 Changed 14 months ago by gh-kliem

  • Branch set to public/27673
  • Commit set to 3bd7a03c38bc78ca938a949fb3483109f3f3978e

New commits:

3bd7a03fixed AttributeError of Polyhedron.volume()

comment:3 Changed 14 months ago by gh-kliem

  • Status changed from new to needs_review

comment:4 Changed 14 months ago by gh-kliem

  • 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: Changed 14 months ago by 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 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 14 months ago by jipilab

  • Cc jipilab added

comment:7 in reply to: ↑ 5 Changed 14 months ago by gh-kliem

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 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:8 Changed 14 months ago by git

  • Commit changed from 3bd7a03c38bc78ca938a949fb3483109f3f3978e to cfa12c74b0387fe8d400454f0c33a691e9d08058

Branch pushed to git repo; I updated commit sha1. New commits:

cfa12c7volume in dimension 0 is counting measure

comment:9 Changed 14 months ago by gh-kliem

  • Description modified (diff)

comment:10 Changed 14 months ago by git

  • Commit changed from cfa12c74b0387fe8d400454f0c33a691e9d08058 to 7995b24ac3eafa0c3c4404257478b67840a48950

Branch pushed to git repo; I updated commit sha1. New commits:

7995b24a bit nicer documentation

comment:11 Changed 13 months ago by jipilab

  • 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 13 months ago by jipilab

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 13 months ago by gh-kliem

  • Description modified (diff)
  • Summary changed from AttributeError of Polyhedron.volume() to Fix Polyhedron.volume() in 0-dimensional space

comment:14 Changed 13 months ago by git

  • Commit changed from 7995b24ac3eafa0c3c4404257478b67840a48950 to 98e05b5d3e5681d7321511c5208d7ce9bfc9bea6

Branch pushed to git repo; I updated commit sha1. New commits:

dd6f981Merge branch 'develop' of git://trac.sagemath.org/sage into public/27673
98e05b5fixed doctest mistake

comment:15 Changed 13 months ago by gh-kliem

  • Status changed from needs_work to needs_review

comment:16 Changed 13 months ago by jipilab

  • 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: Changed 13 months ago by vbraun

  • 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 13 months ago by gh-kliem

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 13 months ago by vbraun

Oh ok, I misread the docstring.

Still a merge conflict though.

comment:20 Changed 13 months ago by git

  • Commit changed from 98e05b5d3e5681d7321511c5208d7ce9bfc9bea6 to 343130cd4a9c32c936e580f3c0e894dae84fb5fa

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

b543c6dremove an obsolete note in schemes, pep cleanup of zariski_vankampen
12373c6Change format of a 'not tested' doc test annotation
8783becleave GMP's spkg-configure.m4 as it was
64f4b4dupdate docs, as discussed on #24905
2fa1373Mention libterm-readkey-perl
406ece2Mention libterm-readline-gnu-perl
a031f09polymake: Mention MOngoDB for polydb
473178epolymake: Update info on macOS
9a4bac1polymake: update MongoDB information for linux distros
343130cUpdated SageMath version to 8.8.beta5

comment:21 Changed 13 months ago by git

  • Commit changed from 343130cd4a9c32c936e580f3c0e894dae84fb5fa to 33b1ff9fd090b5a9272ccf63eacf7f0451e879a8

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

33b1ff9fixed 0-dimensional volume

comment:22 Changed 13 months ago by gh-kliem

  • Status changed from needs_work to needs_review

comment:23 follow-up: Changed 12 months ago by 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.

comment:24 in reply to: ↑ 23 Changed 12 months ago by jipilab

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 12 months ago by jipilab

  • Status changed from needs_review to needs_work

comment:26 Changed 12 months ago by git

  • Commit changed from 33b1ff9fd090b5a9272ccf63eacf7f0451e879a8 to 7a7e57d005962081f1811fcddd74f11278169b65

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7a7e57dvolume in dimension 0 is counting measure

comment:27 Changed 12 months ago by git

  • Commit changed from 7a7e57d005962081f1811fcddd74f11278169b65 to df1f5495b83d0c2ae2cd37478f7138041a5508e9

Branch pushed to git repo; I updated commit sha1. New commits:

df1f549should be back to the original

comment:28 Changed 12 months ago by git

  • Commit changed from df1f5495b83d0c2ae2cd37478f7138041a5508e9 to a7bf147ff3f043167c420e2525ada31068417489

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a7bf147fixed 0-dimensional volume

comment:29 Changed 12 months ago by gh-kliem

  • 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 12 months ago by jipilab

  • Status changed from needs_review to positive_review

Looks good to me now.

comment:31 Changed 12 months ago by vbraun

  • Branch changed from public/27673 to a7bf147ff3f043167c420e2525ada31068417489
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.