[blivet:master] Fixes to Size.humanReadable() method

Vratislav Podzimek vpodzime at redhat.com
Thu Sep 11 08:49:17 UTC 2014


On Fri, 2014-09-05 at 18:49 -0400, mulhern wrote:
> There are several significant changes:
> *) Only quantities that are an exact multiple of the display units are shown
> without a fractional part. Previously some quantities that were not exact
> multiples of the display unit were shown w/out a fractional part.
> *) The places parameter is eliminated entirely.
> *) Rounding is done on a Decimal quantity using quantize() instead of using
> round (which works by conversion to float).
> *) Return the expression that contains the abbreviation rather than the
> prefix unconditionally.
> *) Restrict to binary, not decimal, prefixes and abbreviations, only.
> *) If the number is at least 1024 * the largest available binary prefix,
> use that binary prefix.
> 
> Rationale:
> 
> Sizes are now stored in arbitrary precision Decimal values. The base unit
> of computation is bytes, which is something that can be measured exactly.
> Therefore, the quantities that are measured are precisely known, and,
> barring mistakes, all computations should yield precise values.
> Intuitions from other branches of engineering, where there is an
> imprecision in measurement, or from calculating with float types,
> which have limited precision, do not apply here.
> 
> Anaconda never sets places at all, and it always specifies max_places with
> a keyword. Openlmi does not appear to use humanReadable at all.
> The use of places was barely tested in the existing unittests and its
> purpose is obscure. Removing it simplifies the code a good deal but does
> not itself change any existing behavior (other than in test code) since
> its default was None.
> 
> Since Decimals are arbitrary precision, every digit is significant.
> Rounding should occur on the Decimal value, not by conversion to float.
> All the digits requested by max_places should be displayed, since they
> are all, by definition, significant. For example, the string 64.00 KiB
> indicates that n, the number being displayed, has a value
> within 1/2 of 1% of 1024 of 64 KiB, i.e., 65531 <= n <= 65541 bytes.
> Using quantize() instead of round() also has the practical advantage that
> the actual number of digits after the decimal point in the result will
> always be the same, i.e., max_places. If there is no decimal point,
> because the quantity is known to be an exact multiple of the units, this
> rule will not apply.
> 
> Since 65531 <= 65536 <= 65541 it would be perfectly correct to display
> 65536 bytes as 64.00 KiB. However, since 65536 is exactly 64 KiB it is
> treated specially, and shown with no fractional quantity. The change from
> previous code is that only exact quantities are shown as such. Previously,
> some inexact quantities were also displayed using the format for exact
> quantities.
> 
> abbr is set exactly when prefix is set, and they are only set from an
> array which always contains both an abbr and a prefix. So falling back
> on using the prefix when there is no abbreviation makes no sense.
> On the other hand, it is not wrong to keep the prefix value in the array of
> prefixes, it might make sense to add a use_prefix parameter to humanReadable,
> for example.
> 
> Previously, the code searched among decimal prefixes and abbreviations if it
> could not find a binary prefix that worked.
> We prefer binary prefixes and abbreviations and should use them consistently.
> In addition, our binary prefixes cover a slightly larger range of prefixes
> than our decimal ones, so actually it should not ever be the case that it is
> possible to express a number using a decimal prefix but not with a binary
> prefix. Since we are dealing exclusively with binary prefixes we can use
> 1024 as our limit when searching for the correct prefix, not 1000.
> 
> If the number of bytes is greater than
> 1024 * <factor associated with largest available units> still use the
> largest available units for display. Previously, code treated this case
> as special and displayed the amount as bytes. Reasons for the change are
> that its probably an improvement anyway and it eliminates an extra chunk
> of code that would only be executed in the unlikely event that the number of
> bytes was greater than 1024^9.
> 
> Signed-off-by: mulhern <amulhern at redhat.com>
> ---
>  blivet/size.py     | 101 +++++++++++++++++++++++++----------------------------
>  tests/size_test.py |  58 +++++++++++++++++++++++++-----
>  2 files changed, 98 insertions(+), 61 deletions(-)
First of all, I still don't see a bug anybody not reading the code
noticed and reported. It says "humanReadable", so I think even a string
like "something between 1 GiB and 2 GiB" would do.

>From a mathematical point of view, "15.00 GiB" and "15 GiB" are the same
numbers, so I don't see a reason why we should distinguish between them.
Users don't care about every single byte, they want to see nice numbers.
If their disk is expected to have 120 GiB, they want to see 120 GiB, not
119.99 GiB with a tooltip telling them it is 1blablablabla bytes.

And now for the code...

> 
> diff --git a/blivet/size.py b/blivet/size.py
> index b9f0089..e3e685b 100644
> --- a/blivet/size.py
> +++ b/blivet/size.py
> @@ -30,7 +30,7 @@ from decimal import ROUND_DOWN
>  import six
>  
>  from .errors import SizePlacesError
> -from .i18n import _, P_, N_
> +from .i18n import _, N_
>  
> 
>  # Container for size unit prefix information
> @@ -274,70 +274,65 @@ class Size(Decimal):
>  
>          return None
>  
> -    def humanReadable(self, places=None, max_places=2):
> +    def humanReadable(self, max_places=2):
>          """ Return a string representation of this size with appropriate
> -            size specifier and in the specified number of decimal places
> -            (i.e. the maximal precision is only achieved by setting both places
> -            and max_places to None).
> +            size specifier and in the specified number of decimal places.
> +            Values are always represented using binary not decimal units.
> +            For example, if the number of bytes represented by this size
> +            is 65531, expect the representation to be something like
> +            64.00 KiB, not 65.53 KB.
> +
> +            :param max_places: number of decimal places to use, default is 2
> +            :type max_places: int or NoneType
> +            :returns: a representation of the size
> +            :rtype: str
> +
> +            If max_places is set to None, all non-zero digits will be shown.
> +            Otherwise, max_places digits will be shown.
> +
> +            Single digit or smaller quantities are avoided if possible.
> +            Therefore, 1024.00 KiB is preferred to 1.00 MiB. However,
> +            1 B has no alternative.
> +
> +            Note that a size is an exact quantity of bytes and that Decimal
> +            values are arbitrary precision. Consequently, all numbers after
> +            the decimal point are significant.
>          """
> -        if places is not None and places < 0:
> -            raise SizePlacesError("places= must be >=0 or None")
> -
> -        if max_places is not None and max_places < 0:
> -            raise SizePlacesError("max_places= must be >=0 or None")
> -
> -        in_bytes = int(Decimal(self))
> -        if abs(in_bytes) < 1000:
> -            return "%d %s" % (in_bytes, _("B"))
> -
> -        prev_prefix = None
> -        for prefix_item in _xlated_prefixes():
> -            factor, prefix, abbr = prefix_item
> +        if max_places is not None and (max_places < 0 or not isinstance(max_places, (int, long))):
> +            raise SizePlacesError("max_places must be None or an non-negative integer value")
> +
> +        # Find the smallest prefix which will allow a number less than
> +        # 1024 * 10 to the left of the decimal point. If the number is
> +        # so large that no prefix will satisfy this requirement use the
> +        # largest prefix.
> +        z_pre = _Prefix(1024**0, "".decode("utf-8"), "".decode("utf-8"))
Is there a point in making all people reading this code do a computation
of 1024**0 some of them even wondering what ** exactly does?

> +        for factor, _prefix, abbr in [z_pre] + list(_xlated_binary_prefixes()):
This eliminates the good thing about generators -- we don't have to
construct all the values if we don't need them. By using list() we do.

>              newcheck = super(Size, self).__div__(Decimal(factor))
>  
> -            if abs(newcheck) < 1000:
> +            if abs(newcheck) < 1024 * 10:
>                  # nice value, use this factor, prefix and abbr
>                  break
> -            prev_prefix = prefix_item
> +
> +        # If the size is an exact multiple of the chosen units,
> +        # represent the number as an integer, otherwise represent it with
> +        # a Decimal value rounded to the specified number of places.
> +        if newcheck == int(newcheck):
> +            newcheck = int(newcheck)
>          else:
> -            # no nice value found, just return size in bytes
> -            return "%s %s" % (in_bytes, _("B"))
> -
> -        if abs(newcheck) < 10:
> -            if prev_prefix is not None:
> -                factor, prefix, abbr = prev_prefix # pylint: disable=unpacking-non-sequence
> -                newcheck = super(Size, self).__div__(Decimal(factor))
> -            else:
> -                # less than 10 KiB
> -                return "%s %s" % (in_bytes, _("B"))
> -
> -        retval = newcheck
> -        if places is not None:
> -            retval = round(newcheck, places)
> -
> -        if max_places is not None:
> -            if places is not None:
> -                limit = min((places, max_places))
> -            else:
> -                limit = max_places
> -            retval = round(newcheck, limit)
> -
> -        if retval == int(retval):
> -            # integer value, no point in showing ".0" at the end
> -            retval = int(retval)
> +            if max_places is not None:
> +                newcheck = newcheck.quantize(Decimal(10)** -max_places)
Please add space here                                    ^^^

>  
>          # Format the value with '.' as the decimal separator
> +        # If retval is an int, which will be the case if self is an exact
> +        # multiple of the display units, str(retval) will not contain a '.'.
> +        retval_str = str(newcheck)
> +
>          # If necessary, substitute with a localized separator before returning
> -        retval_str = str(retval)
>          radix = locale.nl_langinfo(locale.RADIXCHAR)
>          if radix != '.':
>              retval_str = retval_str.replace('.', radix)
>  
> -        # abbr and prefix are unicode objects so that lower/upper work correctly
> -        # Convert them to str before concatenating so that the return type is
> -        # str.
> +        # Convert unicode objects to str before concatenating so that the
> +        # resulting expression is a str.
>          # pylint: disable=undefined-loop-variable
> -        if abbr:
> -            return retval_str + " " + abbr.encode("utf-8") + _("B")
> -        else:
> -            return retval_str + " " + prefix.encode("utf-8") + P_("byte", "bytes", newcheck)
> +        return retval_str + " " + abbr.encode("utf-8") + _("B")
> diff --git a/tests/size_test.py b/tests/size_test.py
> index e2288e0..27ffce8 100644
> --- a/tests/size_test.py
> +++ b/tests/size_test.py
> @@ -25,7 +25,6 @@ import unittest
>  
>  import six
>  
> -from blivet.errors import SizePlacesError
>  from blivet.size import Size, _prefixes
>  
>  if six.PY3:
> @@ -37,10 +36,7 @@ class SizeTestCase(unittest.TestCase):
>          self.assertEqual(zero, 0.0)
>  
>          s = Size(500)
> -        with self.assertRaises(SizePlacesError):
> -            s.humanReadable(places=-1)
This should be just replaced with (max_places=-1) and left in place, I
think.

> -
> -        self.assertEqual(s.humanReadable(places=0), "500 B")
> +        self.assertEqual(s.humanReadable(max_places=0), "500 B")
>  
>      def _prefixTestHelper(self, numbytes, factor, prefix, abbr):
>          c = numbytes * factor
> @@ -74,10 +70,10 @@ class SizeTestCase(unittest.TestCase):
>  
>      def testHumanReadable(self):
>          s = Size(long(58929971))
> -        self.assertEquals(s.humanReadable(), "56.2 MiB")
> +        self.assertEquals(s.humanReadable(), "56.20 MiB")
Is this change really to the better for users?

>  
>          s = Size(long(478360371))
> -        self.assertEquals(s.humanReadable(), "456.2 MiB")
> +        self.assertEquals(s.humanReadable(), "456.20 MiB")
>  
>          # humanReable output should be the same as input for big enough sizes
>          # and enough places and integer values
> @@ -102,7 +98,53 @@ class SizeTestCase(unittest.TestCase):
>          s = Size("23.7874 TiB")
>          self.assertEquals(s.humanReadable(max_places=3), "23.787 TiB")
>          s = Size("12.6998 TiB")
> -        self.assertEquals(s.humanReadable(max_places=2), "12.7 TiB")
> +        self.assertEquals(s.humanReadable(max_places=2), "12.70 TiB")
> +
> +        # byte values close to multiples of 2 are shown precisely
> +        s = Size(0xfff)
> +        self.assertEquals(s.humanReadable(max_places=2), "4095 B")
> +        s = Size(8193)
> +        self.assertEquals(s.humanReadable(max_places=2), "8193 B")
> +
> +        # a fractional quantity is shown if the value deviates
> +        # from the whole number of units by more than 1%
> +        s = Size(16384 - (1024/100 + 1))
> +        self.assertEquals(s.humanReadable(max_places=2), "15.99 KiB")
> +
> +        # if max_places is set to None, all digits are displayed
> +        s = Size(0xfffffffffffff)
> +        self.assertEquals(s.humanReadable(max_places=None), "4095.999999999999090505298227 TiB")
> +
> +        # test a very large quantity with no associated abbreviation or prefix
> +        s = Size(1024**9)
> +        self.assertEquals(s.humanReadable(max_places=2), "1024 YiB")
> +        s = Size(1024**9 - 1)
> +        self.assertEquals(s.humanReadable(max_places=2), "1024.00 YiB")
> +        s = Size(1024**9 + 1)
> +        self.assertEquals(s.humanReadable(max_places=2), "1024.00 YiB")
People won't consider those those numbers different, just ugly.

> +        s = Size(1024**10)
> +        self.assertEquals(s.humanReadable(max_places=2), "1048576 YiB")
> +
> +    def testHumanReadableFractionalQuantities(self):
> +        # non-exact quantities are shown as such
> +        s = Size(0xfffffffffffff)
> +        self.assertEquals(s.humanReadable(max_places=2), "4096.00 TiB")
> +        s = Size(0xfffff)
> +        self.assertEquals(s.humanReadable(max_places=2), "1024.00 KiB")
> +        s = Size(0xffff)
> +        self.assertEquals(s.humanReadable(max_places=2), "64.00 KiB")
> +        s = Size(16383)
> +        self.assertEquals(s.humanReadable(max_places=2), "16.00 KiB")
> +        # deviation is less than 1/2 of 1% of 1024
> +        s = Size(16384 - (1024/100/2))
> +        self.assertEquals(s.humanReadable(max_places=2), "16.00 KiB")
> +        # deviation is greater than 1/2 of 1% of 1024
> +        s = Size(16384 - ((1024/100/2) + 1))
> +        self.assertEquals(s.humanReadable(max_places=2), "15.99 KiB")
> +
> +        # exact quantities are shown as exact
> +        s = Size(0x10000000000000)
> +        self.assertEquals(s.humanReadable(max_places=2), "4096 TiB")
>  
>      def testNegative(self):
>          s = Size("-500MiB")

-- 
Vratislav Podzimek

Anaconda Rider | Red Hat, Inc. | Brno - Czech Republic



More information about the anaconda-patches mailing list