Add Quantity types and update formulas to produce typed Samples #422

shsms · 2023-06-05T16:06:37Z

No description provided.

src/frequenz/sdk/timeseries/_quantities.py

leandro-lucarella-frequenz

Lots of comments but this is amazing! Great work! 🎉

src/frequenz/sdk/timeseries/_quantities.py

src/frequenz/sdk/timeseries/_formula_engine/_formula_engine.py

src/frequenz/sdk/timeseries/_formula_engine/_formula_engine_pool.py

src/frequenz/sdk/timeseries/_quantities.py

llucax

Again quite a few comments, but many are things to consider in the future, not something to address in this PR as it is already huge and there is much more value in having it merged soon than in discussing improvements for ages :)

I'm going to make a summary with the main points I see pending:

as_xxx() as properties: I looked at https://google.github.io/styleguide/pyguide.html#213-properties and even when it is vague, it always talks about attributes, and I agree as_xxx is not really an attribute, so I agree to leave it as a function.
default exponent_unit_map provided by Quantity: I still think this is the best option, but if it is controversial, we can leave it for a future PR, specially because it doesn't affect users API.
About moving operations (like __add__) to Quantity: Same as above.
About providing also __mul__ and __truediv__ with other: float: Same as above.
About making Quantity an ABC (and the different alternatives to do this): Also too big of a change for this PR and it doesn't affect the API, so we can do it separately if it makes sense.
Add comments about type: ignore: in this PR.
Disallow the default constructor for types with concrete units (Power(10)): I would address in this PR, as it shouldn't be a big change and affects the API. But could be left for another PR if it turns out not to be that trivial (or controversial)
Using hypothesis: optional but in my experience it helped catching some corner cases. It took me a non-trivial amount of effort to do it right, this is why I wouldn't block the PR on this and has the optional status.
Forbidding .base_value for types with concrete units: ideally in this PR but it might depend on what we decide for Quantity as ABC, so it is also fine to leave to address in the future. Maybe in this PR we can add a note to the doc saying that this should not be used for concrete types.
Exporting/importing TypeVars: we can create a discussion, but in this case in particular I think it shouldn't be exported in timeseries because is not something that users will need often, and if they need to write a generic function or class they can declare the TypeVar themselves.
Internal imports and use concrete unit types in examples: fix in this PR.
Moving window and feature extractor as generic types: fix in this PR (if appropriate).
Clarify __format__ behavior in the docstring: this PR.

For the stuff that you agree we should leave for another PR, can you either create new issues/discussions gathering the info in the relevant comments and then resolve them with a link to the new issue/discussion, or just add a comment saying you agree to leave for another PR and I'll do the issue/discussion creation and resolving.
*

benchmarks/power_distribution/power_distributor.py

llucax · 2023-06-14T10:20:18Z

tests/timeseries/test_quantities.py

+    assert f"{Fz2(1.024445, exponent=-12)}" == "0 Hz"
+
+
+def test_comparison() -> None:


Suggestion: this might be a good candidate to do some property testing using hypothesis.

llucax · 2023-06-14T10:21:47Z

benchmarks/timeseries/periodic_feature_extractor.py

@@ -34,7 +35,7 @@ async def init_feature_extractor(period: int) -> PeriodicFeatureExtractor:
        timedelta(seconds=1), lm_chan.new_receiver(), timedelta(seconds=1)
    )

-    await lm_chan.new_sender().send(Sample(datetime.now(tz=timezone.utc), 0))
+    await lm_chan.new_sender().send(Sample(datetime.now(tz=timezone.utc), Quantity(0)))


BTW, this could be explored in a followup PR (if we believe it might work and be better, I don't think it is worth to address it now).

llucax · 2023-06-14T10:24:12Z

examples/power_distribution.py

@@ -126,7 +127,9 @@ async def run(self) -> None:
                if (datetime.now(timezone.utc) - time_data).total_seconds() > 30:
                    _logger.error("Active power data are stale")
                    continue
-                queue.put_nowait(active_power.value)
+                queue.put_nowait(
+                    None if not active_power.value else active_power.value.base_value


Looking at this, I think it might be better to leave base_value as private too, in code like this it is not clear at all what base_value is, is it Watt? Kilowatt? active_power.value.as_watts() is much more clear.

The base_value is really just for the generic cases described here: #422 (comment)

And for the dimensionless cases like reading from the resampler directly.

I guess the power_distribution example is no longer relevant anyway, because people should use the battery pool instead. I have an in-progress branch for that, so won't forget. But will do it after this one is merged.

The issue I see about it is it can be used in sub-classes, completely by-passing the unit checks. This is why it would be nice (if we don't go with #450) to find some way to make it available only in the base unit-less Quantity.

pyproject.toml

src/frequenz/sdk/timeseries/_quantities.py

llucax · 2023-06-14T11:24:45Z

tests/timeseries/test_periodic_feature_extractor.py

As the moving window, shouldn't this be parametrized on a QuantityT instead of using a unit-less Quantity?

It turns out MovingWindow etc can't propagate types because the most likely output types for these classes is a numpy array. So it doesn't have to be generic, just needs to accept any thing that gets put in here. And using the base Quantity type achieves that.

#450 (comment)

llucax · 2023-06-14T12:02:23Z

tests/timeseries/test_quantities.py

+    assert power.as_megawatts() == 0.0012
+    assert power == Power.from_milliwatts(1200000.0)
+    assert power == Power.from_megawatts(0.0012)
+    assert power != Power.from_watts(1000.0)


I was referring more about the case where the type system sees both types as Any, but I'm not sure if it's relevant anymore, because the type system is not rejecting Fz1(1.024445, exponent=0) < Fz2(1.024445, exponent=3) anyways.

But still testing that operations fails between different unit it is still probably worth testing.

src/frequenz/sdk/timeseries/_quantities.py

This check is redundant because it is a type-check that's already taken care of by mypy. Signed-off-by: Sahas Subramanian <[email protected]>

Signed-off-by: Sahas Subramanian <[email protected]>

This makes the transition to a generic `Sample` type easier, which is done in a subsequent commit. This commit also introduces the `output_type` parameter to formula builders and formula engines. The new parameter is used to determine which type of quantity instance a formula engine would produce. Signed-off-by: Sahas Subramanian <[email protected]>

Signed-off-by: Sahas Subramanian <[email protected]>

This commit also updates the generated formulas to use the right generic types like Power and Current, for power and current formulas respectively. Signed-off-by: Sahas Subramanian <[email protected]>

The previous design used a single dictionary `_engines` to store engines of different types. This is fixed now. Signed-off-by: Sahas Subramanian <[email protected]>

Signed-off-by: Sahas Subramanian <[email protected]>

llucax

OK, I created a couple of discussions:

Feel free to close all the discussions where I point to either of those 2 discussions.

The remaining points are all minor and non-blockers to me, so I will approve. If you decide not to go with the disabled __init__, please create a follow-up issue so we don't forget.

benchmarks/power_distribution/power_distributor.py

llucax · 2023-06-19T15:23:33Z

tests/timeseries/test_quantities.py

+    assert power.as_megawatts() == 0.0012
+    assert power == Power.from_milliwatts(1200000.0)
+    assert power == Power.from_megawatts(0.0012)
+    assert power != Power.from_watts(1000.0)


Oh, OK. Can't you just use a function that takes Any as arguments instead to test this and circumvent the type system to check the exception is raised? I think it makes it more clear what are you testing for, because using adding random # type: ignore shouldn't be something we really care about. But having the type relaxed to be Any is a real world case. Maybe you can even leave your example as is and just type-hint variables with any:

fz1: Any = Fz1(1.024445, exponent=0) fz2: Any = Fz2(1.024445, exponent=3) with pytest.raises(TypeError) as excinfo: assert fz1 < fz2 assert ( excinfo.value.args[0] == "'<' not supported between instances of 'Fz1' and 'Fz2'" )

llucax · 2023-06-19T15:29:06Z

src/frequenz/sdk/timeseries/_quantities.py

@@ -309,6 +310,55 @@ def __sub__(self, other: Self) -> Self:
        """
        return self.from_watts(self.as_watts() - other.as_watts())

+    def __mul__(self, duration: timedelta) -> "Energy":


Sure. This is all I mean:

def __truediv__(self, other: Current | Voltage | float) -> Voltage | Current | Power: if isinstance(other, Current): return Voltage(self._base_value / other._base_value, exponent=0) if isinstance(other, Voltage): return Current(self._base_value / other._base_value, exponent=0) if isinstance(other, numbers.Real): # Only 1 extra `isinstance()` for all reals (`int` and `float` and whatever user-defined type that inherits from `numbers.Real`) return type(self)(self._base_value / other, exponent=0) raise TypeError( f"unsupported operand type(s) for /: '{type(self)}' and '{type(other)}'" )

>>> import numbers >>> isinstance(1, numbers.Real) True >>> isinstance(1.0, numbers.Real) True

llucax · 2023-06-19T15:36:24Z

benchmarks/timeseries/periodic_feature_extractor.py

@@ -34,7 +35,7 @@ async def init_feature_extractor(period: int) -> PeriodicFeatureExtractor:
        timedelta(seconds=1), lm_chan.new_receiver(), timedelta(seconds=1)
    )

-    await lm_chan.new_sender().send(Sample(datetime.now(tz=timezone.utc), 0))
+    await lm_chan.new_sender().send(Sample(datetime.now(tz=timezone.utc), Quantity(0)))


I still don't understand why you need the unit-less Quantity type, if you need it as a step to pass it to numpy or something like that, then you can decompose into a float already, right? But maybe we can discuss in a follow up discussion:

Explore if `Quantity` can be made an abstract class #450

llucax · 2023-06-19T15:40:28Z

examples/power_distribution.py

@@ -126,7 +127,9 @@ async def run(self) -> None:
                if (datetime.now(timezone.utc) - time_data).total_seconds() > 30:
                    _logger.error("Active power data are stale")
                    continue
-                queue.put_nowait(active_power.value)
+                queue.put_nowait(
+                    None if not active_power.value else active_power.value.base_value


The issue I see about it is it can be used in sub-classes, completely by-passing the unit checks. This is why it would be nice (if we don't go with #450) to find some way to make it available only in the base unit-less Quantity.

llucax · 2023-06-19T15:42:28Z

src/frequenz/sdk/timeseries/_moving_window.py

@@ -132,7 +133,7 @@ async def run() -> None:
    def __init__(  # pylint: disable=too-many-arguments
        self,
        size: timedelta,
-        resampled_data_recv: Receiver[Sample],
+        resampled_data_recv: Receiver[Sample[Quantity]],


The link doesn't seem to be pointing to any file:line in particular, maybe you can mention the file:line in the comment so I can look for it?

llucax · 2023-06-19T15:50:01Z

src/frequenz/sdk/timeseries/_quantities.py

+    @property
+    def base_value(self) -> float:
+        """Return the value of this quantity in the base unit.
+
+        Returns:
+            The value of this quantity in the base unit.
+        """
+        return self._base_value


how about as_base_unit instead? Because unitless is not clear that there's no transformations going on.

Maybe, I'm not sure if it is as discouraging. I think the root problem for me is you can have a Quantity without a unit in general and this is just a small consequence of that, so maybe we don't focus on this detail until we have the major design topic sorted out. By now I'm pretty sure I'm missing some fundamental use case that needs this. Maybe you can clarify it in a call and continue the conversation in #450).

llucax · 2023-06-19T15:54:05Z

tests/timeseries/test_periodic_feature_extractor.py

#450 (comment)

llucax · 2023-06-19T16:05:26Z

src/frequenz/sdk/timeseries/_quantities.py

@@ -309,6 +310,55 @@ def __sub__(self, other: Self) -> Self:
        """
        return self.from_watts(self.as_watts() - other.as_watts())

+    def __mul__(self, duration: timedelta) -> "Energy":


https://github.com/frequenz-floss/frequenz-sdk-python/discussions/451

github-actions bot added part:actor Affects an actor ot the actors utilities (decorator, etc.) part:data-pipeline Affects the data pipeline part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests labels Jun 5, 2023

shsms commented Jun 5, 2023

View reviewed changes

src/frequenz/sdk/timeseries/_quantities.py Show resolved Hide resolved

shsms assigned christianparpart, matthias-wende-frequenz, ela-kotulska-frequenz, stefan-brus-frequenz and shsms and unassigned christianparpart, matthias-wende-frequenz, ela-kotulska-frequenz and stefan-brus-frequenz Jun 6, 2023

shsms commented Jun 6, 2023

View reviewed changes

src/frequenz/sdk/timeseries/_quantities.py Outdated Show resolved Hide resolved

shsms added this to the v0.22.0 milestone Jun 6, 2023

leandro-lucarella-frequenz reviewed Jun 6, 2023

View reviewed changes

src/frequenz/sdk/timeseries/_quantities.py Outdated Show resolved Hide resolved

leandro-lucarella-frequenz reviewed Jun 6, 2023

View reviewed changes

src/frequenz/sdk/timeseries/_quantities.py Show resolved Hide resolved

shsms force-pushed the type-wrappers branch from c1fe897 to 9c4ce00 Compare June 7, 2023 18:15

github-actions bot added the part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) label Jun 7, 2023

shsms force-pushed the type-wrappers branch 3 times, most recently from 015b3ba to 15f4900 Compare June 12, 2023 09:00

shsms mentioned this pull request Jun 12, 2023

Remove obsolete uses of typing.* #433

Closed

shsms marked this pull request as ready for review June 12, 2023 12:40

shsms requested a review from a team as a code owner June 12, 2023 12:40

shsms requested review from llucax and leandro-lucarella-frequenz and removed request for leandro-lucarella-frequenz June 12, 2023 12:40

shsms force-pushed the type-wrappers branch from 15f4900 to 5c159c8 Compare June 12, 2023 13:09

llucax mentioned this pull request Jun 14, 2023

Disable pylint's unsubscriptable-object check frequenz-floss/frequenz-repo-config-python#54

Closed

llucax reviewed Jun 14, 2023

View reviewed changes

shsms force-pushed the type-wrappers branch from 5c159c8 to 7a4b29b Compare June 19, 2023 10:08

shsms mentioned this pull request Jun 18, 2024

Use base SI unit prefixes in custom Quantities when no custom prefixes are provided frequenz-floss/frequenz-quantities-python#8

Open

shsms added 15 commits June 19, 2023 12:32

Disable buggy pylint unsubscriptable-object check

3dc2187

This check is redundant because it is a type-check that's already taken care of by mypy. Signed-off-by: Sahas Subramanian <[email protected]>

Add a base Quantity class

1519cb3

Signed-off-by: Sahas Subramanian <[email protected]>

Add class for representing Power quantities

81f9c61

Signed-off-by: Sahas Subramanian <[email protected]>

Add class for representing Current quantities

04c077f

Signed-off-by: Sahas Subramanian <[email protected]>

Add class for representing Voltage quantities

160780f

Signed-off-by: Sahas Subramanian <[email protected]>

Add class for representing Energy quantities

6c4621f

Signed-off-by: Sahas Subramanian <[email protected]>

Add methods for composing quantities with each other

a8d1545

Signed-off-by: Sahas Subramanian <[email protected]>

Add a generic type representing all quantity implementations

de33b03

Signed-off-by: Sahas Subramanian <[email protected]>

Make Samples generic over Quantity types

ba1bec5

Signed-off-by: Sahas Subramanian <[email protected]>

Make FormulaEngines generic over Quantity types

e00cc91

This commit also updates the generated formulas to use the right generic types like Power and Current, for power and current formulas respectively. Signed-off-by: Sahas Subramanian <[email protected]>

Update FormulaEnginePool to have multiple strongly-typed functions

d6580af

The previous design used a single dictionary `_engines` to store engines of different types. This is fixed now. Signed-off-by: Sahas Subramanian <[email protected]>

Update BatteryPool's control methods to accept/require Power objects

4ce90a2

Signed-off-by: Sahas Subramanian <[email protected]>

Make the new Quantity types part of the public API

b509bfd

Signed-off-by: Sahas Subramanian <[email protected]>

Document reasons for # type: ignore in _formula_engine.py

22a47b6

Signed-off-by: Sahas Subramanian <[email protected]>

shsms force-pushed the type-wrappers branch from 7a4b29b to 22a47b6 Compare June 19, 2023 14:07

shsms requested a review from llucax June 19, 2023 14:09

llucax approved these changes Jun 19, 2023

View reviewed changes

shsms mentioned this pull request Jun 20, 2023

Disable default constructor for derived Quantity types #452

Closed

shsms added this pull request to the merge queue Jun 20, 2023

Merged via the queue into frequenz-floss:v0.x.x with commit 5c44da2 Jun 20, 2023

shsms deleted the type-wrappers branch June 20, 2023 12:18

llucax mentioned this pull request Jun 20, 2023

Add initial implementation of quantity types #389

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Quantity types and update formulas to produce typed Samples #422

Add Quantity types and update formulas to produce typed Samples #422

shsms commented Jun 5, 2023

leandro-lucarella-frequenz left a comment

llucax left a comment

llucax Jun 14, 2023

llucax Jun 14, 2023

llucax Jun 14, 2023

shsms Jun 16, 2023

llucax Jun 19, 2023

llucax Jun 14, 2023

shsms Jun 19, 2023

llucax Jun 19, 2023

llucax Jun 14, 2023

llucax left a comment •

edited

Loading

llucax Jun 19, 2023

llucax Jun 19, 2023

llucax Jun 19, 2023

llucax Jun 19, 2023

llucax Jun 19, 2023

llucax Jun 19, 2023

llucax Jun 19, 2023

llucax Jun 19, 2023

		assert f"{Fz2(1.024445, exponent=-12)}" == "0 Hz"


		def test_comparison() -> None:

Add Quantity types and update formulas to produce typed Samples #422

Add Quantity types and update formulas to produce typed Samples #422

Conversation

shsms commented Jun 5, 2023

leandro-lucarella-frequenz left a comment

Choose a reason for hiding this comment

llucax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llucax left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llucax left a comment •

edited

Loading