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

mulhern amulhern at redhat.com
Fri Sep 5 22:49:56 UTC 2014


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(-)

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"))
+        for factor, _prefix, abbr in [z_pre] + list(_xlated_binary_prefixes()):
             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)
 
         # 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)
-
-        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")
 
         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")
+        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")
-- 
1.9.3



More information about the anaconda-patches mailing list