Skip to content

Allow multiplying Quantity by float #875

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Feb 15, 2024
Merged

Allow multiplying Quantity by float #875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f78393f
Allow all quantities multiplication by float | Percentage
llucax Feb 8, 2024
7c754a6
Improve `type: ignore` for `__mul__`
llucax Feb 13, 2024
a4e879a
Use hypothesis for `__mul__` `Percentage` tests
llucax Feb 13, 2024
7eaee36
Add `__mul__()` tests for `float`
llucax Feb 13, 2024
5118985
Update release notes
llucax Feb 13, 2024
9d45a46
Apply suggestions from code review
llucax Feb 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion RELEASE_NOTES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

## New Features

<!-- Here goes the main new features and examples or instructions on how to use them -->
- Allow multiplying any `Quantity` by a `float` too. This just scales the `Quantity` value.

## Bug Fixes

Expand Down
172 changes: 128 additions & 44 deletions src/frequenz/sdk/timeseries/_quantities.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,19 @@ def __sub__(self, other: Self) -> Self:
difference._base_value = self._base_value - other._base_value
return difference

def __mul__(self, percent: Percentage) -> Self:
@overload
def __mul__(self, scalar: float, /) -> Self:
"""Scale this quantity by a scalar.

Args:
scalar: The scalar by which to scale this quantity.

Returns:
The scaled quantity.
"""

@overload
def __mul__(self, percent: Percentage, /) -> Self:
"""Scale this quantity by a percentage.

Args:
Expand All @@ -343,12 +355,23 @@ def __mul__(self, percent: Percentage) -> Self:
Returns:
The scaled quantity.
"""
if not isinstance(percent, Percentage):
return NotImplemented

product = type(self).__new__(type(self))
product._base_value = self._base_value * percent.as_fraction()
return product
def __mul__(self, value: float | Percentage, /) -> Self:
"""Scale this quantity by a scalar or percentage.

Args:
value: The scalar or percentage by which to scale this quantity.

Returns:
The scaled quantity.
"""
match value:
case float():
return type(self)._new(self._base_value * value)
case Percentage():
return type(self)._new(self._base_value * value.as_fraction())
case _:
return NotImplemented

def __gt__(self, other: Self) -> bool:
"""Return whether this quantity is greater than another.
Expand Down Expand Up @@ -583,19 +606,38 @@ def as_megawatts(self) -> float:
"""
return self._base_value / 1e6

@overload # type: ignore
def __mul__(self, other: Percentage) -> Self:
# We need the ignore here because otherwise mypy will give this error:
# > Overloaded operator methods can't have wider argument types in overrides
# The problem seems to be when the other type implements an **incompatible**
# __rmul__ method, which is not the case here, so we should be safe.
# Please see this example:
# https://github.com/python/mypy/blob/c26f1297d4f19d2d1124a30efc97caebb8c28616/test-data/unit/check-overloading.test#L4738C1-L4769C55
# And a discussion in a mypy issue here:
# https://github.com/python/mypy/issues/4985#issuecomment-389692396
@overload # type: ignore[override]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to add some more context, when Marenz was adding these, we had no idea what the issue was, because without these, mypy kept crashing. Looks like they've fixed that bug and now we are able to understand the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I didn't get a crash (thankfully!) but I still had to do my research, because the error message is not very clear why is there a problem with doing that.

def __mul__(self, scalar: float, /) -> Self:
"""Scale this power by a scalar.

Args:
scalar: The scalar by which to scale this power.

Returns:
The scaled power.
"""

@overload
def __mul__(self, percent: Percentage, /) -> Self:
"""Scale this power by a percentage.

Args:
other: The percentage by which to scale this power.
percent: The percentage by which to scale this power.

Returns:
The scaled power.
"""

@overload
def __mul__(self, other: timedelta) -> Energy:
def __mul__(self, other: timedelta, /) -> Energy:
"""Return an energy from multiplying this power by the given duration.

Args:
Expand All @@ -605,23 +647,22 @@ def __mul__(self, other: timedelta) -> Energy:
The calculated energy.
"""

def __mul__(self, other: Percentage | timedelta) -> Self | Energy:
def __mul__(self, other: float | Percentage | timedelta, /) -> Self | Energy:
"""Return a power or energy from multiplying this power by the given value.

Args:
other: The percentage or duration to multiply by.
other: The scalar, percentage or duration to multiply by.

Returns:
A power or energy.
"""
if isinstance(other, Percentage):
return super().__mul__(other)
if isinstance(other, timedelta):
return Energy.from_watt_hours(
self._base_value * other.total_seconds() / 3600.0
)

return NotImplemented
match other:
case float() | Percentage():
return super().__mul__(other)
case timedelta():
return Energy._new(self._base_value * other.total_seconds() / 3600.0)
case _:
return NotImplemented

@overload
def __truediv__(self, other: Current) -> Voltage:
Expand Down Expand Up @@ -725,19 +766,31 @@ def as_milliamperes(self) -> float:
"""
return self._base_value * 1e3

@overload # type: ignore
def __mul__(self, other: Percentage) -> Self:
# See comment for Power.__mul__ for why we need the ignore here.
@overload # type: ignore[override]
def __mul__(self, scalar: float, /) -> Self:
"""Scale this current by a scalar.

Args:
scalar: The scalar by which to scale this current.

Returns:
The scaled current.
"""

@overload
def __mul__(self, percent: Percentage, /) -> Self:
"""Scale this current by a percentage.

Args:
other: The percentage by which to scale this current.
percent: The percentage by which to scale this current.

Returns:
The scaled current.
"""

@overload
def __mul__(self, other: Voltage) -> Power:
def __mul__(self, other: Voltage, /) -> Power:
"""Multiply the current by a voltage to get a power.

Args:
Expand All @@ -747,21 +800,22 @@ def __mul__(self, other: Voltage) -> Power:
The calculated power.
"""

def __mul__(self, other: Percentage | Voltage) -> Self | Power:
def __mul__(self, other: float | Percentage | Voltage, /) -> Self | Power:
"""Return a current or power from multiplying this current by the given value.

Args:
other: The percentage or voltage to multiply by.
other: The scalar, percentage or voltage to multiply by.

Returns:
A current or power.
"""
if isinstance(other, Percentage):
return super().__mul__(other)
if isinstance(other, Voltage):
return Power.from_watts(self._base_value * other._base_value)

return NotImplemented
match other:
case float() | Percentage():
return super().__mul__(other)
case Voltage():
return Power._new(self._base_value * other._base_value)
case _:
return NotImplemented


class Voltage(
Expand Down Expand Up @@ -840,19 +894,31 @@ def as_kilovolts(self) -> float:
"""
return self._base_value / 1e3

@overload # type: ignore
def __mul__(self, other: Percentage) -> Self:
# See comment for Power.__mul__ for why we need the ignore here.
@overload # type: ignore[override]
def __mul__(self, scalar: float, /) -> Self:
"""Scale this voltage by a scalar.

Args:
scalar: The scalar by which to scale this voltage.

Returns:
The scaled voltage.
"""

@overload
def __mul__(self, percent: Percentage, /) -> Self:
"""Scale this voltage by a percentage.

Args:
other: The percentage by which to scale this voltage.
percent: The percentage by which to scale this voltage.

Returns:
The scaled voltage.
"""

@overload
def __mul__(self, other: Current) -> Power:
def __mul__(self, other: Current, /) -> Power:
"""Multiply the voltage by the current to get the power.

Args:
Expand All @@ -862,21 +928,22 @@ def __mul__(self, other: Current) -> Power:
The calculated power.
"""

def __mul__(self, other: Percentage | Current) -> Self | Power:
def __mul__(self, other: float | Percentage | Current, /) -> Self | Power:
"""Return a voltage or power from multiplying this voltage by the given value.

Args:
other: The percentage or current to multiply by.
other: The scalar, percentage or current to multiply by.

Returns:
The calculated voltage or power.
"""
if isinstance(other, Percentage):
return super().__mul__(other)
if isinstance(other, Current):
return Power.from_watts(self._base_value * other._base_value)

return NotImplemented
match other:
case float() | Percentage():
return super().__mul__(other)
case Current():
return Power._new(self._base_value * other._base_value)
case _:
return NotImplemented


class Energy(
Expand Down Expand Up @@ -959,6 +1026,23 @@ def as_megawatt_hours(self) -> float:
"""
return self._base_value / 1e6

def __mul__(self, other: float | Percentage) -> Self:
"""Scale this energy by a percentage.

Args:
other: The percentage by which to scale this energy.

Returns:
The scaled energy.
"""
match other:
case float():
return self._new(self._base_value * other)
case Percentage():
return self._new(self._base_value * other.as_fraction())
case _:
return NotImplemented

@overload
def __truediv__(self, other: timedelta) -> Power:
"""Return a power from dividing this energy by the given duration.
Expand Down
92 changes: 62 additions & 30 deletions tests/timeseries/test_quantities.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,31 +544,69 @@ def test_abs() -> None:
assert abs(-pct) == Percentage.from_fraction(30)


def test_quantity_multiplied_with_precentage() -> None:
@pytest.mark.parametrize("quantity_ctor", _QUANTITY_CTORS + [Quantity])
# Use a small amount to avoid long running tests, we have too many combinations
@hypothesis.settings(max_examples=10)
@hypothesis.given(
quantity_value=st.floats(
allow_infinity=False,
allow_nan=False,
allow_subnormal=False,
# We need to set this because otherwise constructors with big exponents will
# cause the value to be too big for the float type, and the test will fail.
max_value=1e298,
min_value=-1e298,
),
percent=st.floats(allow_infinity=False, allow_nan=False, allow_subnormal=False),
)
def test_quantity_multiplied_with_precentage(
quantity_ctor: type[Quantity], quantity_value: float, percent: float
) -> None:
"""Test the multiplication of all quantities with percentage."""
percentage = Percentage.from_percent(50)
power = Power.from_watts(1000.0)
voltage = Voltage.from_volts(230.0)
current = Current.from_amperes(2)
energy = Energy.from_kilowatt_hours(12)
percentage_ = Percentage.from_percent(50)

assert power * percentage == Power.from_watts(500.0)
assert voltage * percentage == Voltage.from_volts(115.0)
assert current * percentage == Current.from_amperes(1)
assert energy * percentage == Energy.from_kilowatt_hours(6)
assert percentage_ * percentage == Percentage.from_percent(25)

power *= percentage
assert power == Power.from_watts(500.0)
voltage *= percentage
assert voltage == Voltage.from_volts(115.0)
current *= percentage
assert current == Current.from_amperes(1)
energy *= percentage
assert energy == Energy.from_kilowatt_hours(6)
percentage_ *= percentage
assert percentage_ == Percentage.from_percent(25)
percentage = Percentage.from_percent(percent)
quantity = quantity_ctor(quantity_value)
expected_value = quantity.base_value * (percent / 100.0)
print(f"{quantity=}, {percentage=}, {expected_value=}")

product = quantity * percentage
print(f"{product=}")
assert product.base_value == expected_value

quantity *= percentage
print(f"*{quantity=}")
assert quantity.base_value == expected_value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to retain the print statements? don't they pollute the test output when trying to debug something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are useful when something fails. I guess if you are focusing on some other test you can always run pytest test_other.py::my_interesting_test if you want to keep the noise down. If someone wants to remove it in the future, I'm also fine with that, as if one gets a failure is not that hard to add the print() (except maybe if it is only happening in the CI).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well there's always:

assert quantity.base_value == expected_value, f"{quantity=} * {percentage=} != {expected_value}! 🙀 "



@pytest.mark.parametrize("quantity_ctor", _QUANTITY_CTORS + [Quantity])
# Use a small amount to avoid long running tests, we have too many combinations
@hypothesis.settings(max_examples=10)
@hypothesis.given(
quantity_value=st.floats(
allow_infinity=False,
allow_nan=False,
allow_subnormal=False,
# We need to set this because otherwise constructors with big exponents will
# cause the value to be too big for the float type, and the test will fail.
max_value=1e298,
min_value=-1e298,
),
scalar=st.floats(allow_infinity=False, allow_nan=False, allow_subnormal=False),
)
def test_quantity_multiplied_with_float(
quantity_ctor: type[Quantity], quantity_value: float, scalar: float
) -> None:
"""Test the multiplication of all quantities with a float."""
quantity = quantity_ctor(quantity_value)
expected_value = quantity.base_value * scalar
print(f"{quantity=}, {expected_value=}")

product = quantity * scalar
print(f"{product=}")
assert product.base_value == expected_value

quantity *= scalar
print(f"*{quantity=}")
assert quantity.base_value == expected_value


def test_invalid_multiplications() -> None:
Expand Down Expand Up @@ -602,12 +640,6 @@ def test_invalid_multiplications() -> None:
with pytest.raises(TypeError):
energy *= quantity # type: ignore

for quantity in [power, voltage, current, energy, Percentage.from_percent(50)]:
with pytest.raises(TypeError):
_ = quantity * 200.0 # type: ignore
with pytest.raises(TypeError):
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
Expand Down