-
Notifications
You must be signed in to change notification settings - Fork 4
Make quantities generic over float
and Decimal
#61
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
base: v1.x.x
Are you sure you want to change the base?
Conversation
This will no-longer be easy once we support decimals as the base value. Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Support for decimals can be added if required. Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
These will be used in the decimal tests as well. Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR makes the quantities module generic over float and Decimal, ensuring that operations and conversions work correctly for both numeric types. Key changes include updating type annotations and constructors to use generics, revising test modules to reflect the new generic usage, and adjusting marshmallow handling for generic quantity fields.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/utils.py | Introduces generic Fz1/Fz2 subclasses and updates sanity-check constants. |
tests/test_quantities.py | Updates type annotations and parametrizations to use the generic type. |
tests/experimental/test_marshmallow.py | Adjusts field definitions and assertions to account for generic quantities. |
tests/init.py | Adds basic module initialization and license header. |
src/frequenz/quantities/experimental/marshmallow.py | Revises marshmallow field types to expect Quantity[float]. |
src/frequenz/quantities/_voltage.py | Updates constructors and arithmetic operations for generic types. |
src/frequenz/quantities/_temperature.py | Modifies functions to use BaseValueT for temperature conversions. |
src/frequenz/quantities/_reactive_power.py | Adjusts type hints and arithmetic operations for reactive power quantities. |
src/frequenz/quantities/_quantity.py | Refactors the Quantity base class to be generic over float and Decimal. |
src/frequenz/quantities/_power.py | Updates creation and operations for power quantities using generics. |
src/frequenz/quantities/_percentage.py | Revises methods to use the generic BaseValueT type. |
src/frequenz/quantities/_frequency.py | Updates frequency creation and conversion functions to be generic. |
src/frequenz/quantities/_energy.py | Revises energy conversions and operations to use generics. |
src/frequenz/quantities/_current.py | Adjusts current quantity methods and arithmetic to support generics. |
src/frequenz/quantities/_apparent_power.py | Updates apparent power construction and conversions to use generics. |
Comments suppressed due to low confidence (1)
src/frequenz/quantities/_quantity.py:43
- When using Decimal as the underlying type, converting ints to float may lead to unintended loss of precision; consider converting ints using the generic type's constructor (e.g., Decimal(value)) to preserve the intended type.
value = float(value)
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider correcting the spelling of '_SANITFY_NUM_CLASSES' to '_SANITIZE_NUM_CLASSES' for improved clarity.
_SANITFY_NUM_CLASSES = 7 | |
_SANITIZE_NUM_CLASSES = 7 |
Copilot uses AI. Check for mistakes.
It is possible to improve the typing for the various constructors using overrides, will get to that and documentation fixes, if there isn't much resistance to this changes. |
This would be a replacement of #62, right? |
Are there other uses of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general looks good to me.
About the breaking changes, I guess that's the removal of zero()
, which seems to be work-aroundable (if caching is too complicated we can also not cache it anymore, but I think having zero()
is very handy, otherwise you need to use one of the Power.from_watt(0.0)
). And the other breaking change is now you need to QuantityT
-> QuantityT[float]
for old code, right?
Maybe we could call the generic type QuantityBase
and then make @deprecated Quantity: TypeAlias = QuantityBase[float]
.
The problem with this is we'll need to change the code first to be QuantityBase[float]
and then at some other point change it back to Quantity[float]
if we want to make it backwards compatible.
But if we don't go this route, it might be challenging to do the upgrade, as we'll need to upgrade all repositories at the same time. This approach allows us to have a smooth transition over a longer period of time.
BUT...
I'm still wondering if we really need this? Do we really need a Power[Decimal]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you just parametrize the original test to test both float
and decimal
? I guess precision could be an issue, but I guess at these tests we don't care that much about precision, so maybe the test can convert values to float
(so decimal is converted to float before checking) and use pytest.approx()
.
_zero_cache: dict[type, Quantity] = {} | ||
"""Cache for zero singletons. | ||
|
||
This is a workaround for mypy getting confused when using @functools.cache and | ||
@classmethod combined with returning Self. It believes the resulting type of this | ||
method is Self and complains that members of the actual class don't exist in Self, | ||
so we need to implement the cache ourselves. | ||
""" | ||
|
||
@classmethod | ||
def zero(cls) -> Self: | ||
"""Return a quantity with value 0.0. | ||
|
||
Returns: | ||
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.0 | ||
cls._zero_cache[cls] = _zero | ||
assert isinstance(_zero, cls) | ||
return _zero | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get why is keeping zero()
(and its cache) is a problem. Can't you just have _zero_cache: dict[tuple[type, type], Quantity] = {}
and then...
I thought it was going to be easier, but it got tricky, although with the help of Gemini I could end up with this code that works:
from decimal import Decimal
from typing import Generic, TypeVar, Self, Dict, Type
# 1. Define the TypeVar as before
BaseValueT = TypeVar("BaseValueT", float, Decimal)
class Quantity(Generic[BaseValueT]):
_base_value: BaseValueT
# 2. The cache is on the base class. The key is now just the type (float or Decimal).
_zero_cache: Dict[Type[BaseValueT], 'Quantity'] = {}
def __init__(self, value: BaseValueT):
self._base_value = value
def __repr__(self) -> str:
return f"Quantity(value={self._base_value!r}, type={type(self._base_value).__name__})"
# 3. Implement __class_getitem__ to capture the type argument
def __class_getitem__(cls, item: Type[BaseValueT]):
"""
This method is called when doing Quantity[float] or Quantity[Decimal].
It's the key to solving the problem.
"""
# First, let the default Generic machinery create the specialized alias
# (e.g., the Quantity[float] type object).
generic_alias = super().__class_getitem__(item)
# Now, "tag" that new specialized alias with the type it was created with.
# We'll use this tag in the zero() method.
generic_alias._type_arg = item
return generic_alias
@classmethod
def zero(cls) -> Self:
"""Return a cached singleton quantity with a value of 0."""
# 4. In the classmethod, access the tag we attached in __class_getitem__.
# This reliably gives us `float` or `Decimal`.
# We add a fallback to the first constraint for the raw Quantity.zero() call.
try:
base_value_type = cls._type_arg
except AttributeError:
# This happens if called on the raw Quantity class, not Quantity[T]
base_value_type = BaseValueT.__constraints__[0]
# 5. Use the correct type as the key for the cache.
_zero = cls._zero_cache.get(base_value_type)
if _zero is None:
# The instance we create should be of the class `cls` was called on.
# However, the underlying value `_base_value` must be of the correct type.
_zero = cls.__new__(cls)
_zero._base_value = base_value_type(0)
cls._zero_cache[base_value_type] = _zero
return _zero
# --- DEMONSTRATION ---
print("--- Running final corrected code with __class_getitem__ ---")
# When Quantity[float] is accessed, __class_getitem__ runs and tags the type.
# Then .zero() is called on that tagged type.
zero_float = Quantity[float].zero()
print(f"Zero for float: {zero_float}")
zero_decimal = Quantity[Decimal].zero()
print(f"Zero for decimal: {zero_decimal}")
print("-" * 20)
# Subsequent calls correctly return the cached instances
zero_float_2 = Quantity[float].zero()
zero_decimal_2 = Quantity[Decimal].zero()
print(f"Is the float zero a singleton? {zero_float is zero_float_2}")
print(f"Is the decimal zero a singleton? {zero_decimal is zero_decimal_2}")
print(f"Are the float and decimal zeros different objects? {zero_float is not zero_decimal}")
print("-" * 20)
# The cache is now correctly populated.
print("Contents of _zero_cache:")
for key, value in Quantity._zero_cache.items():
print(f" Key: {key!r} -> Value: {value!r}")
# Calling on the raw class will default to the first type (float)
zero_raw = Quantity.zero()
print(f"\nZero for raw class: {zero_raw}")
print(f"Is raw zero the same as float zero? {zero_raw is zero_float}")
--- Running final corrected code with __class_getitem__ ---
Zero for float: Quantity(value=0.0, type=float)
Zero for decimal: Quantity(value=Decimal('0'), type=Decimal)
--------------------
Is the float zero a singleton? True
Is the decimal zero a singleton? True
Are the float and decimal zeros different objects? True
--------------------
Contents of _zero_cache:
Key: <class 'float'> -> Value: Quantity(value=0.0, type=float)
Key: <class 'decimal.Decimal'> -> Value: Quantity(value=Decimal('0'), type=Decimal)
Zero for raw class: Quantity(value=Decimal('0'), type=Decimal)
Is raw zero the same as float zero? False
Note
We probably need to add back cls
to the key and make it a tuple, this code example is not taking into account subclasses
Yes, the trading API uses a decimal to represent power everywhere: https://github.com/frequenz-floss/frequenz-api-common/blob/ddb3147996194ae85421a8498d7f098e878eaf78/proto/frequenz/api/common/v1/market/power.proto#L27-L30 |
return Power._new(self._base_value / (other.total_seconds() / 3600.0)) | ||
return Power._new( | ||
self._base_value | ||
/ self._base_value.__class__(other.total_seconds() / 3600.0) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't you use Decimals before division?
Instead of Decimal(other.total_seconds() / 3600.0)
-> Decimal(other.total_seconds()) / Decimal(3600)
No description provided.