Skip to content

Commit

Permalink
API: always recompute the volume in polytope.polytope.volume
Browse files Browse the repository at this point in the history
because this is implicit. Explicit is better than implicit [PEP 20][pep20].
Doing so surprises the user, and is a silent error in case the `Polytope`
or `Region` has been modified by the user since the last time its
volume was computed. Errors should never pass silently [PEP 20][pep20].

The attribute `volume` of the classes `Polytope` and `Region` is
a property that is cached in the attribute `_volume`. Therefore,
users know and expect that accessing this attribute will return
a cached value.

However, what is nonobvious to users, and would be unexpected,
is that also when calling the function `polytope.polytope.volume`,
still the polytope's cached value is returned, if present.
A user would expect that calling the function `volume` would
compute the volume again, because that is an explicit call to the
volume computation algorithm.

More fundamentally, without this change, it becomes impossible
for a user to recompute a polytope's volume without resetting
the attribute `_volume` to `None` in user code. This would require
that users use internal package details for updating the volume.

Do log a debug-level message for tracking the computation.

The `try..except` was a bug, because it masked an `Exception`.
There is no reason why `polyreg` would not have `_volume` as attribute
(both `Polytope` and `Region` have such an attribute).
Even if `polyreg` was required to have an attribute named `_volume`,
still catching `Exception` is too broad. Instead, `AttributeError`
would be caught.

[pep20]: https://www.python.org/dev/peps/pep-0020/
  • Loading branch information
johnyf committed May 28, 2021
1 parent 2bd0a5b commit bfc6b0b
Showing 1 changed file with 2 additions and 5 deletions.
7 changes: 2 additions & 5 deletions polytope/polytope.py
Original file line number Diff line number Diff line change
Expand Up @@ -1477,11 +1477,8 @@ def volume(polyreg, nsamples=None):
"""
if not is_fulldim(polyreg):
return 0.0
try:
if polyreg._volume is not None:
return polyreg._volume
except Exception:
logger.debug('computing volume...')
if polyreg._volume is not None:
logger.debug('recomputing polytope volume...')
# `Region` ?
if isinstance(polyreg, Region):
tot_vol = 0.
Expand Down

0 comments on commit bfc6b0b

Please sign in to comment.