Make sure base_value is always float (#874)

In Python `float` is a bit weird, because `isinstance(1, float)` is
`False` but still `int` is a subtype of `float` in terms of the type
system, so `int` will be accepted when a `float` is specified in
arguments for example.

This leads to quantities that can have `int` as base value if it was
constructed with a `int` instead of a `float`, which is unintuitive when
the `base_value` type hint is `float` and can potentially cause
problems, in particular when addressing #821.

Author: llucax

RELEASE_NOTES.md

@@ -15,3 +15,4 @@
 ## Bug Fixes
 
 - Fix grid current formula generator to add the operator `+` to the engine only when the component category is handled.
+- Fix bug where sometimes the `base_value` of a `Quantity` could be of a different type than `float`.

src/frequenz/sdk/timeseries/_quantities.py

@@ -48,7 +48,22 @@ def __init__(self, value: float, exponent: int = 0) -> None:
             value: The value of this quantity in a given exponent of the base unit.
             exponent: The exponent of the base unit the given value is in.
         """
-        self._base_value = value * 10**exponent
+        self._base_value = value * 10.0**exponent
+
+    @classmethod
+    def _new(cls, value: float, *, exponent: int = 0) -> Self:
+        """Instantiate a new quantity subclass instance.
+
+        Args:
+            value: The value of this quantity in a given exponent of the base unit.
+            exponent: The exponent of the base unit the given value is in.
+
+        Returns:
+            A new quantity subclass instance.
+        """
+        self = cls.__new__(cls)
+        self._base_value = value * 10.0**exponent
+        return self
 
     def __init_subclass__(cls, exponent_unit_map: dict[int, str]) -> None:
         """Initialize a new subclass of Quantity.
@@ -78,15 +93,15 @@ def __init_subclass__(cls, exponent_unit_map: dict[int, str]) -> None:
 
     @classmethod
     def zero(cls) -> Self:
-        """Return a quantity with value 0.
+        """Return a quantity with value 0.0.
 
         Returns:
-            A quantity with value 0.
+            A quantity with value 0.0.
         """
         _zero = cls._zero_cache.get(cls, None)
         if _zero is None:
             _zero = cls.__new__(cls)
-            _zero._base_value = 0
+            _zero._base_value = 0.0
             cls._zero_cache[cls] = _zero
         assert isinstance(_zero, cls)
         return _zero
@@ -464,9 +479,7 @@ def from_celsius(cls, value: float) -> Self:
         Returns:
             A new temperature quantity.
         """
-        power = cls.__new__(cls)
-        power._base_value = value
-        return power
+        return cls._new(value)
 
     def as_celsius(self) -> float:
         """Return the temperature in degrees Celsius.
@@ -508,9 +521,7 @@ def from_watts(cls, watts: float) -> Self:
         Returns:
             A new power quantity.
         """
-        power = cls.__new__(cls)
-        power._base_value = watts
-        return power
+        return cls._new(watts)
 
     @classmethod
     def from_milliwatts(cls, milliwatts: float) -> Self:
@@ -522,9 +533,7 @@ def from_milliwatts(cls, milliwatts: float) -> Self:
         Returns:
             A new power quantity.
         """
-        power = cls.__new__(cls)
-        power._base_value = milliwatts * 10**-3
-        return power
+        return cls._new(milliwatts, exponent=-3)
 
     @classmethod
     def from_kilowatts(cls, kilowatts: float) -> Self:
@@ -536,9 +545,7 @@ def from_kilowatts(cls, kilowatts: float) -> Self:
         Returns:
             A new power quantity.
         """
-        power = cls.__new__(cls)
-        power._base_value = kilowatts * 10**3
-        return power
+        return cls._new(kilowatts, exponent=3)
 
     @classmethod
     def from_megawatts(cls, megawatts: float) -> Self:
@@ -550,9 +557,7 @@ def from_megawatts(cls, megawatts: float) -> Self:
         Returns:
             A new power quantity.
         """
-        power = cls.__new__(cls)
-        power._base_value = megawatts * 10**6
-        return power
+        return cls._new(megawatts, exponent=6)
 
     def as_watts(self) -> float:
         """Return the power in watts.
@@ -690,9 +695,7 @@ def from_amperes(cls, amperes: float) -> Self:
         Returns:
             A new current quantity.
         """
-        current = cls.__new__(cls)
-        current._base_value = amperes
-        return current
+        return cls._new(amperes)
 
     @classmethod
     def from_milliamperes(cls, milliamperes: float) -> Self:
@@ -704,9 +707,7 @@ def from_milliamperes(cls, milliamperes: float) -> Self:
         Returns:
             A new current quantity.
         """
-        current = cls.__new__(cls)
-        current._base_value = milliamperes * 10**-3
-        return current
+        return cls._new(milliamperes, exponent=-3)
 
     def as_amperes(self) -> float:
         """Return the current in amperes.
@@ -789,9 +790,7 @@ def from_volts(cls, volts: float) -> Self:
         Returns:
             A new voltage quantity.
         """
-        voltage = cls.__new__(cls)
-        voltage._base_value = volts
-        return voltage
+        return cls._new(volts)
 
     @classmethod
     def from_millivolts(cls, millivolts: float) -> Self:
@@ -803,9 +802,7 @@ def from_millivolts(cls, millivolts: float) -> Self:
         Returns:
             A new voltage quantity.
         """
-        voltage = cls.__new__(cls)
-        voltage._base_value = millivolts * 10**-3
-        return voltage
+        return cls._new(millivolts, exponent=-3)
 
     @classmethod
     def from_kilovolts(cls, kilovolts: float) -> Self:
@@ -817,9 +814,7 @@ def from_kilovolts(cls, kilovolts: float) -> Self:
         Returns:
             A new voltage quantity.
         """
-        voltage = cls.__new__(cls)
-        voltage._base_value = kilovolts * 10**3
-        return voltage
+        return cls._new(kilovolts, exponent=3)
 
     def as_volts(self) -> float:
         """Return the voltage in volts.
@@ -914,9 +909,7 @@ def from_watt_hours(cls, watt_hours: float) -> Self:
         Returns:
             A new energy quantity.
         """
-        energy = cls.__new__(cls)
-        energy._base_value = watt_hours
-        return energy
+        return cls._new(watt_hours)
 
     @classmethod
     def from_kilowatt_hours(cls, kilowatt_hours: float) -> Self:
@@ -928,9 +921,7 @@ def from_kilowatt_hours(cls, kilowatt_hours: float) -> Self:
         Returns:
             A new energy quantity.
         """
-        energy = cls.__new__(cls)
-        energy._base_value = kilowatt_hours * 10**3
-        return energy
+        return cls._new(kilowatt_hours, exponent=3)
 
     @classmethod
     def from_megawatt_hours(cls, megawatt_hours: float) -> Self:
@@ -942,9 +933,7 @@ def from_megawatt_hours(cls, megawatt_hours: float) -> Self:
         Returns:
             A new energy quantity.
         """
-        energy = cls.__new__(cls)
-        energy._base_value = megawatt_hours * 10**6
-        return energy
+        return cls._new(megawatt_hours, exponent=6)
 
     def as_watt_hours(self) -> float:
         """Return the energy in watt hours.
@@ -1039,9 +1028,7 @@ def from_hertz(cls, hertz: float) -> Self:
         Returns:
             A new frequency quantity.
         """
-        frequency = cls.__new__(cls)
-        frequency._base_value = hertz
-        return frequency
+        return cls._new(hertz)
 
     @classmethod
     def from_kilohertz(cls, kilohertz: float) -> Self:
@@ -1053,9 +1040,7 @@ def from_kilohertz(cls, kilohertz: float) -> Self:
         Returns:
             A new frequency quantity.
         """
-        frequency = cls.__new__(cls)
-        frequency._base_value = kilohertz * 10**3
-        return frequency
+        return cls._new(kilohertz, exponent=3)
 
     @classmethod
     def from_megahertz(cls, megahertz: float) -> Self:
@@ -1067,9 +1052,7 @@ def from_megahertz(cls, megahertz: float) -> Self:
         Returns:
             A new frequency quantity.
         """
-        frequency = cls.__new__(cls)
-        frequency._base_value = megahertz * 10**6
-        return frequency
+        return cls._new(megahertz, exponent=6)
 
     @classmethod
     def from_gigahertz(cls, gigahertz: float) -> Self:
@@ -1081,9 +1064,7 @@ def from_gigahertz(cls, gigahertz: float) -> Self:
         Returns:
             A new frequency quantity.
         """
-        frequency = cls.__new__(cls)
-        frequency._base_value = gigahertz * 10**9
-        return frequency
+        return cls._new(gigahertz, exponent=9)
 
     def as_hertz(self) -> float:
         """Return the frequency in hertz.
@@ -1152,9 +1133,7 @@ def from_percent(cls, percent: float) -> Self:
         Returns:
             A new percentage quantity.
         """
-        percentage = cls.__new__(cls)
-        percentage._base_value = percent
-        return percentage
+        return cls._new(percent)
 
     @classmethod
     def from_fraction(cls, fraction: float) -> Self:
@@ -1166,9 +1145,7 @@ def from_fraction(cls, fraction: float) -> Self:
         Returns:
             A new percentage quantity.
         """
-        percentage = cls.__new__(cls)
-        percentage._base_value = fraction * 100
-        return percentage
+        return cls._new(fraction * 100)
 
     def as_percent(self) -> float:
         """Return this quantity as a percentage.

tests/timeseries/test_quantities.py

@@ -3,12 +3,15 @@
 
 """Tests for quantity types."""
 
+import inspect
 from datetime import timedelta
+from typing import Callable
 
 import hypothesis
 import pytest
 from hypothesis import strategies as st
 
+from frequenz.sdk.timeseries import _quantities
 from frequenz.sdk.timeseries._quantities import (
     Current,
     Energy,
@@ -45,6 +48,48 @@ class Fz2(
     """Frequency quantity with broad exponent unit map."""
 
 
+_CtorType = Callable[[float], Quantity]
+
+# This is the current number of subclasses. This probably will get outdated, but it will
+# provide at least some safety against something going really wrong and end up testing
+# an empty list. With this we should at least make sure we are not testing less classes
+# than before. We don't get the actual number using len(_QUANTITY_SUBCLASSES) because it
+# would defeat the purpose of the test.
+_SANITFY_NUM_CLASSES = 7
+
+_QUANTITY_SUBCLASSES = [
+    cls
+    for _, cls in inspect.getmembers(
+        _quantities,
+        lambda m: inspect.isclass(m) and issubclass(m, Quantity) and m is not Quantity,
+    )
+]
+
+# A very basic sanity check that are messing up the introspection
+assert len(_QUANTITY_SUBCLASSES) >= _SANITFY_NUM_CLASSES
+
+_QUANTITY_BASE_UNIT_STRINGS = [
+    cls._new(0).base_unit  # pylint: disable=protected-access
+    for cls in _QUANTITY_SUBCLASSES
+]
+for unit in _QUANTITY_BASE_UNIT_STRINGS:
+    assert unit is not None
+
+_QUANTITY_CTORS = [
+    method
+    for cls in _QUANTITY_SUBCLASSES
+    for _, method in inspect.getmembers(
+        cls,
+        lambda m: inspect.ismethod(m)
+        and m.__name__.startswith("from_")
+        and m.__name__ != ("from_string"),
+    )
+]
+# A very basic sanity check that are messing up the introspection. There are actually
+# many more constructors than classes, but this still works as a very basic check.
+assert len(_QUANTITY_CTORS) >= _SANITFY_NUM_CLASSES
+
+
 def test_zero() -> None:
     """Test the zero value for quantity."""
     assert Quantity(0.0) == Quantity.zero()
@@ -101,6 +146,31 @@ def test_zero() -> None:
     assert Percentage.zero() is Percentage.zero()  # It is a "singleton"
 
 
+@pytest.mark.parametrize("quantity_ctor", _QUANTITY_CTORS)
+def test_base_value_from_ctor_is_float(quantity_ctor: _CtorType) -> None:
+    """Test that the base value always is a float."""
+    quantity = quantity_ctor(1)
+    assert isinstance(quantity.base_value, float)
+
+
+@pytest.mark.parametrize("quantity_type", _QUANTITY_SUBCLASSES + [Quantity])
+def test_base_value_from_zero_is_float(quantity_type: type[Quantity]) -> None:
+    """Test that the base value always is a float."""
+    quantity = quantity_type.zero()
+    assert isinstance(quantity.base_value, float)
+
+
+@pytest.mark.parametrize(
+    "quantity_type, unit", zip(_QUANTITY_SUBCLASSES, _QUANTITY_BASE_UNIT_STRINGS)
+)
+def test_base_value_from_string_is_float(
+    quantity_type: type[Quantity], unit: str
+) -> None:
+    """Test that the base value always is a float."""
+    quantity = quantity_type.from_string(f"1 {unit}")
+    assert isinstance(quantity.base_value, float)
+
+
 def test_string_representation() -> None:
     """Test the string representation of the quantities."""
     assert str(Quantity(1.024445, exponent=0)) == "1.024"
@@ -539,6 +609,9 @@ def test_invalid_multiplications() -> None:
             quantity *= 200.0  # type: ignore
 
 
+# We can't use _QUANTITY_TYPES here, because it will break the tests, as hypothesis
+# will generate more values, some of which are unsupported by the quantities. See the
+# test comment for more details.
 @pytest.mark.parametrize("quantity_type", [Power, Voltage, Current, Energy, Frequency])
 @pytest.mark.parametrize("exponent", [0, 3, 6, 9])
 @hypothesis.settings(
@@ -563,8 +636,14 @@ def test_to_and_from_string(
     generation and regenerate it with the more appropriate unit and precision.
     """
     quantity = quantity_type.__new__(quantity_type)
-    # pylint: disable=protected-access
-    quantity._base_value = value * 10**exponent
+    quantity._base_value = value * 10**exponent  # pylint: disable=protected-access
+    # The above should be replaced with:
+    # quantity = quantity_type._new(  # pylint: disable=protected-access
+    #     value, exponent=exponent
+    # )
+    # But we can't do that now, because, you guessed it, it will also break the tests
+    # (_new() will use 10.0**exponent instead of 10**exponent, which seems to have some
+    # effect on the tests.
     quantity_str = f"{quantity:.{exponent}}"
     from_string = quantity_type.from_string(quantity_str)
     try: