Skip to content
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.

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

Parse Quantity Strings #824

Merged
merged 4 commits into from
Feb 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions RELEASE_NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@

- The `actor.ChannelRegistry` is now type-aware.

- A new class method `Quantity.from_string()` has been added to allow the creation of `Quantity` objects from strings.

## Bug Fixes

- 0W power requests are now not adjusted to exclusion bounds by the `PowerManager` and `PowerDistributor`, and are sent over to the microgrid API directly.
Expand All @@ -87,3 +89,5 @@
A bug made the resampler interpret zero values as `None` when generating new samples, so if the result of the resampling is zero, the resampler would just produce `None` values.

- The PowerManager no longer holds on to proposals from dead actors forever. If an actor hasn't sent a new proposal in 60 seconds, the available proposal from that actor is dropped.

- Fix `Quantity.__format__()` sometimes skipping the number for very small values.
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ dev-pytest = [
"async-solipsism == 0.5",
# For checking docstring code examples
"frequenz-sdk[dev-examples]",
"hypothesis == 6.92.1",
]
dev = [
"frequenz-sdk[dev-mkdocs,dev-flake8,dev-formatting,dev-mkdocs,dev-mypy,dev-noxfile,dev-pylint,dev-pytest]",
Expand Down
65 changes: 61 additions & 4 deletions src/frequenz/sdk/timeseries/_quantities.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,42 @@ def zero(cls) -> Self:
assert isinstance(_zero, cls)
return _zero

@classmethod
def from_string(cls, string: str) -> Self:
"""Return a quantity from a string representation.
Args:
string: The string representation of the quantity.
Returns:
A quantity object with the value given in the string.
Raises:
ValueError: If the string does not match the expected format.
"""
split_string = string.split(" ")

if len(split_string) != 2:
raise ValueError(
f"Expected a string of the form 'value unit', got {string}"
)

assert cls._exponent_unit_map is not None
exp_map = cls._exponent_unit_map

for exponent, unit in exp_map.items():
if unit == split_string[1]:
instance = cls.__new__(cls)
try:
instance._base_value = float(split_string[0]) * 10**exponent
except ValueError as error:
raise ValueError(f"Failed to parse string '{string}'.") from error

return instance

raise ValueError(f"Unknown unit {split_string[1]}")

@property
def base_value(self) -> float:
"""Return the value of this quantity in the base unit.
Expand Down Expand Up @@ -163,6 +199,7 @@ def __str__(self) -> str:
"""
return self.__format__("")

# pylint: disable=too-many-branches
def __format__(self, __format_spec: str) -> str:
"""Return a formatted string representation of this quantity.
Expand Down Expand Up @@ -214,8 +251,22 @@ def __format__(self, __format_spec: str) -> str:
if math.isinf(self._base_value) or math.isnan(self._base_value):
return f"{self._base_value} {self._exponent_unit_map[0]}"

abs_value = abs(self._base_value)
exponent = math.floor(math.log10(abs_value)) if abs_value else 0
if abs_value := abs(self._base_value):
precision_pow = 10 ** (precision)
# Prevent numbers like 999.999999 being rendered as 1000 V
# instead of 1 kV.
# This could happen because the str formatting function does
# rounding as well.
# This is an imperfect solution that works for _most_ cases.
# isclose parameters were chosen according to the observed cases
if math.isclose(abs_value, precision_pow, abs_tol=1e-4, rel_tol=0.01):
# If the value is close to the precision, round it
exponent = math.ceil(math.log10(precision_pow))
else:
exponent = math.floor(math.log10(abs_value))
else:
exponent = 0

unit_place = exponent - exponent % 3
if unit_place < min(self._exponent_unit_map):
unit = self._exponent_unit_map[min(self._exponent_unit_map.keys())]
Expand All @@ -225,11 +276,17 @@ def __format__(self, __format_spec: str) -> str:
unit_place = max(self._exponent_unit_map)
else:
unit = self._exponent_unit_map[unit_place]

value_str = f"{self._base_value / 10 ** unit_place:.{precision}f}"
stripped = value_str.rstrip("0").rstrip(".")

if value_str in ("-0", "0"):
stripped = value_str
else:
stripped = value_str.rstrip("0").rstrip(".")

if not keep_trailing_zeros:
value_str = stripped
unit_str = unit if stripped != "0" else self._exponent_unit_map[0]
unit_str = unit if stripped not in ("-0", "0") else self._exponent_unit_map[0]
return f"{value_str} {unit_str}"

def __add__(self, other: Self) -> Self:
Expand Down
51 changes: 49 additions & 2 deletions tests/timeseries/test_quantities.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@

from datetime import timedelta

import hypothesis
import pytest
from hypothesis import strategies as st

from frequenz.sdk.timeseries._quantities import (
Current,
Expand Down Expand Up @@ -105,8 +107,11 @@ def test_string_representation() -> None:
assert (
repr(Quantity(1.024445, exponent=0)) == "Quantity(value=1.024445, exponent=0)"
)
assert f"{Quantity(0.50001, exponent=0):.0}" == "1"
assert f"{Quantity(1.024445, exponent=0)}" == "1.024"
assert f"{Quantity(1.024445, exponent=0):.0}" == "1"
assert f"{Quantity(0.124445, exponent=0):.0}" == "0"
assert f"{Quantity(0.50001, exponent=0):.0}" == "1"
assert f"{Quantity(1.024445, exponent=0):.6}" == "1.024445"

assert f"{Quantity(1.024445, exponent=3)}" == "1024.445"
Expand All @@ -128,7 +133,6 @@ def test_string_representation() -> None:

assert f"{Fz1(1.024445, exponent=6)}" == "1024.445 kHz"
assert f"{Fz2(1.024445, exponent=6)}" == "1.024 MHz"

assert f"{Fz1(1.024445, exponent=9)}" == "1024445 kHz"
assert f"{Fz2(1.024445, exponent=9)}" == "1.024 GHz"

Expand All @@ -147,6 +151,12 @@ def test_string_representation() -> None:
assert f"{Fz1(-20)}" == "-20 Hz"
assert f"{Fz1(-20000)}" == "-20 kHz"

assert f"{Power.from_watts(0.000124445):.0}" == "0 W"
assert f"{Energy.from_watt_hours(0.124445):.0}" == "0 Wh"
assert f"{Power.from_watts(-0.0):.0}" == "-0 W"
assert f"{Power.from_watts(0.0):.0}" == "0 W"
assert f"{Voltage.from_volts(999.9999850988388)}" == "1 kV"


def test_isclose() -> None:
"""Test the isclose method of the quantities."""
Expand Down Expand Up @@ -355,7 +365,7 @@ def test_frequency() -> None:
"""Test the frequency class."""
freq = Frequency.from_hertz(0.0000002)
assert f"{freq:.9}" == "0.0000002 Hz"
freq = Frequency.from_kilohertz(600000.0)
freq = Frequency.from_kilohertz(600_000.0)
assert f"{freq}" == "600 MHz"

freq = Frequency.from_hertz(6.0)
Expand Down Expand Up @@ -527,3 +537,40 @@ def test_invalid_multiplications() -> None:
_ = quantity * 200.0 # type: ignore
with pytest.raises(TypeError):
quantity *= 200.0 # type: ignore


@pytest.mark.parametrize("quantity_type", [Power, Voltage, Current, Energy, Frequency])
@pytest.mark.parametrize("exponent", [0, 3, 6, 9])
@hypothesis.settings(
max_examples=1000
) # Set to have a decent amount of examples (default is 100)
@hypothesis.seed(42) # Seed that triggers a lot of problematic edge cases
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really want to leave the seed fixed? Isn't it more useful to let it change to potentially catch more random cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know for a fact that there are more cases and solving those requires an unreasonable amount of work (at this point).
@sahas-subramanian-frequenz mentioned an imperfect solution is better than no solution at all and at least here I would agree.

There will be cases were it prints slightly weird, maybe using a sub-optimal unit etc, but the printed result will never be straight out wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that sounds completely reasonable and I agree 100%. Can you add a comment summarizing this here? Because for the uninformed casual reader it will be really hard to figure all of this out, they'll probably just try to remove those two lines and be completely puzzled about why everything breaks now. I know future me will attempt that 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to be a pain in the ass about this, but I mean something more like "we are setting these fixed because there are still corner cases that will fail if we allow hypothesis to do random tests, this should be removed when all the corner cases are properly handled".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You did see the larger docstring of the function explaining basically that in other words?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, sorry, I expected it in the comments instead of the docstring so my brain stopped parsing after I couldn't find the clarification there.

@hypothesis.given(value=st.floats(min_value=-1.0, max_value=1.0))
def test_to_and_from_string(
quantity_type: type[Quantity], exponent: int, value: float
) -> None:
"""Test string parsing and formatting.

The parameters for this test are constructed to stay deterministic.

With a different (or random) seed or different max_examples the
test will show failing examples.

Fixing those cases was considered an unreasonable amount of work
at the time of writing.

For the future, one idea was to parse the string number after the first
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_str = f"{quantity:.{exponent}}"
from_string = quantity_type.from_string(quantity_str)
try:
assert f"{from_string:.{exponent}}" == quantity_str
except AssertionError as error:
pytest.fail(
f"Failed for {quantity.base_value} != from_string({from_string.base_value}) "
+ f"with exponent {exponent} and source value '{value}': {error}"
)
Loading