Add helper function to calculate energy metric #93

flora-hofmann-frequenz · 2024-09-03T09:25:42Z

We have a client request to read out energy metrics. Since the inverter / meter readings are rather unreliable, we want to add a helper function to calculate the energy metric.

flora-hofmann-frequenz · 2024-09-03T09:28:32Z

@llucax: This is a very first idea on how to include a helper function to the base client in reporting. Would you please add your input.

llucax · 2024-09-03T09:29:41Z

pyproject.toml

  "grpcio-tools >= 1.54.2, < 2",
  "protobuf >= 4.25.3, < 6",
  "frequenz-client-base[grpcio] >= 0.5.0, < 0.6.0",
+  "pandas == 2.2.2"


Do we really want to make an API client depend on pandas? For me this sounds like it should be put in a higher level.

No, not at all. It was just a first shot as I am unsure of the data structure in which we want to supply values. I am happy to move away from pandas. The best option may also be connected to data structure that we will use when exposing the states @cwasicki?

Agree, we shouldn't use pandas if not urgently required.

llucax

OK, so in terms of code structure, I would make the helper function a free function instead of a class method. And probably would take an Iterator or Sequence instead of a list if that's an option.

noah-kreutzer-frequenz · 2024-09-03T13:50:07Z

@flora-hofmann-frequenz just for my understanding, what happens if there is a gap in data for the power?

noah-kreutzer-frequenz · 2024-09-03T14:08:36Z

I also think that the customer would rather expect the total amount of energy that was consumed or delivered as output, rather than the energy for every timestamp. Something like a "Zählerstand" or at least the difference between the "Zählerstände"

flora-hofmann-frequenz · 2024-09-03T14:40:24Z

I also think that the customer would rather expect the total amount of energy that was consumed or delivered as output, rather than the energy for every timestamp. Something like a "Zählerstand" or at least the difference between the "Zählerstände"

@noah-kreutzer-frequenz can you please make an example for this? Isn't the total amount dependent on a certain timeframe?

noah-kreutzer-frequenz · 2024-09-03T15:05:44Z

I also think that the customer would rather expect the total amount of energy that was consumed or delivered as output, rather than the energy for every timestamp. Something like a "Zählerstand" or at least the difference between the "Zählerstände"

@noah-kreutzer-frequenz can you please make an example for this? Isn't the total amount dependent on a certain timeframe?

So rather than a Timeseries the looking like this

time	energy	power related to the time frame
0:00	0.0	0
1:00	1.0	1
2:00	2.0	2
3:00	0.0	2
4:00	4.0	4
5:00	0.0	0

I think something like this is what the customer would expect

time	total energy	power related to the time frame
0:00	0.0	0
1:00	1.0	1
2:00	3.0	2
3:00	3.0	0
4:00	7.0	4
5:00	7.0	0

or even only the the last value

noah-kreutzer-frequenz · 2024-09-03T15:06:57Z

and potentially the possoiblity of seperating between consumed energy and delivered energy, so they don't cancel out :))

cwasicki · 2024-09-04T12:50:34Z

pyproject.toml

  "grpcio-tools >= 1.54.2, < 2",
  "protobuf >= 4.25.3, < 6",
  "frequenz-client-base[grpcio] >= 0.5.0, < 0.6.0",
+  "pandas == 2.2.2"


Agree, we shouldn't use pandas if not urgently required.

cwasicki · 2024-09-04T13:02:05Z

src/frequenz/client/reporting/_client.py

+        # Convert the timedelta to hours
+        hours = time_interval.total_seconds() / 3600.0
+
+        energy_wh_series = ac_active_power_series * hours


I don't think this is what is wanted. Usually the energy metric is integrated over time, so the power would need to be converted to energy and then cumulatively added.

cwasicki · 2024-09-04T13:03:30Z

src/frequenz/client/reporting/_client.py

+        metric_samples: list[MetricSample], time_interval: timedelta
+    ) -> pd.Series:
+        """
+        Calculate the energy metric with AC_ACTIVE_POWER values and time interval.


I don't remember whether this should have been from power or the actual energy metrics. For latter, the only thing that needs to be done is to remove the resets. That could be achieved by taking the diff, removing the negatives (reset) and cumulatively sum them up again.

cwasicki · 2024-09-04T13:05:51Z

src/frequenz/client/reporting/_client.py

+
+    @staticmethod
+    def calculate_energy_metric(
+        metric_samples: list[MetricSample], time_interval: timedelta


I think this should be a wrapper around list_microgrid_components_data with the same arguments but the metric, which is given.

cwasicki · 2024-09-04T13:06:51Z

src/frequenz/client/reporting/_client.py

            print(f"RPC failed: {e}")
            return
+
+    @staticmethod


Good that this is staticmethod, I see this as part of a tooling on top of the client and not the client itself. For now that would be overkill though.

cwasicki · 2024-09-04T13:14:22Z

src/frequenz/client/reporting/_client.py

            return
+
+    @staticmethod
+    def calculate_energy_metric(


Needs a clearer name, maybe cumulative_energy, total_energy, integrated_power` or so, depending on which approach we go for.

Signed-off-by: Flora <[email protected]>

Signed-off-by: flora-hofmann-frequenz <[email protected]>

flora-hofmann-frequenz · 2024-09-10T10:22:23Z

@cwasicki & @noah-kreutzer-frequenz please take a look at the energy metric logic again. I was trying to implement the following:

consumption = sum(
    e2 - e1 for e1, e2 in zip(filtered_samples, filtered_samples[1:])
    if e2 - e1 > 0
)

production = sum(
    e2 - e1 for e1, e2 in zip(filtered_samples, filtered_samples[1:])
    if e2 - e1 < 0
)

Alongside the previous comments and the option to use power or energy metrics (in case energy is not available).

cwasicki · 2024-09-10T12:46:48Z

src/frequenz/client/reporting/_client.py

+    values = [
+        sample.value
+        for sample in metric_samples
+        if start_time <= sample.timestamp <= end_time


Shouldn't happen anyway, but typically exclusive timestamps for the end time are used, i.e. < end_time.

Why do we actually need to extract the values again and not just loop over the metric_samples (see example for power).

cwasicki · 2024-09-10T14:54:01Z

src/frequenz/client/reporting/_client.py

+        if use_active_power:
+            # Convert power to energy if using AC_ACTIVE_POWER
+            time_interval_hours = (
+                resolution or (end_time - start_time).total_seconds()


The second part looks off to me since each power needs to be integrated over it's time period to energy, so would only work for given resolution. You could just use:

consumption = sum( m1.value * (m2.timestamp-m1.timestamp).total_seconds() for m in zip(metric_samples, metric_samples[1:] if m1.value > 0) / 3600.0

cwasicki · 2024-09-10T14:56:58Z

src/frequenz/client/reporting/_client.py

+
+
+# pylint: disable=too-many-arguments
+async def cumulative_energy(


Maybe this should be part of the client for now, otherwise it could be a separate module.

cwasicki · 2024-09-10T15:00:38Z

src/frequenz/client/reporting/_client.py

+
+# pylint: disable=too-many-arguments
+async def cumulative_energy(
+    client: ReportingApiClient,


If it's not part of the client, it needs an URL and key. I guess part of the client makes things simpler now.

@llucax Any thoughts on this? This helper function IMO goes beyond a thin client and could be seen as additional tooling on top of the client. We do think it is overkill to start that now though and wait for further cases.

If it's not part of the client, it needs an URL and key. I guess part of the client makes things simpler now.

No, it just needs a client, which you should have already instantiated. It really exactly the same, is just a syntax issue. The method would be async def cumulative_energy(self: ReportingApiClient,...) the same, it is just called self by convention. You can actually also call a method like this ReportingApiClient.cumulative_energy(client, ...).

I like utilities being separated because it keeps the client interface clear, otherwise is really hard to draw a line between what should be part of the client what shouldn't.

@llucax Any thoughts on this? This helper function IMO goes beyond a thin client and could be seen as additional tooling on top of the client. We do think it is overkill to start that now though and wait for further cases.

I think we should adopt the https://github.com/frequenz-floss/frequenz-dispatch-python approach if there is a room for a higher level interface. At least IMHO that worked out well. In dispatch we basically have the https://github.com/frequenz-floss/frequenz-client-dispatch-python as a thin wrapper over the API and then frequenz-dispatch-python provides a higher level interface, which makes a lot of assumptions about how the API should be used, so it is much less flexible, but much easier to use for 99% of the cases (scientifically proven :P).

Okay, I think it will be best to set up frequenz-reporting-python and move the energy metric there :-)

flora-hofmann-frequenz · 2024-09-12T10:24:58Z

This PR will be closed as it moved to this PR in a new repo.

flora-hofmann-frequenz requested a review from cwasicki September 3, 2024 09:25

flora-hofmann-frequenz self-assigned this Sep 3, 2024

flora-hofmann-frequenz requested a review from llucax September 3, 2024 09:25

github-actions bot added part:docs Affects the documentation part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) labels Sep 3, 2024

llucax reviewed Sep 3, 2024

View reviewed changes

cwasicki reviewed Sep 4, 2024

View reviewed changes

flora-hofmann-frequenz force-pushed the helper_energy branch from 2a8ef4f to 024624d Compare September 10, 2024 10:11

Add helper function to calculate energy metric

1a1d7c1

Signed-off-by: Flora <[email protected]>

flora-hofmann-frequenz force-pushed the helper_energy branch from 024624d to 1a1d7c1 Compare September 10, 2024 10:14

flora-hofmann-frequenz requested a review from cwasicki September 10, 2024 10:15

Merge branch 'v0.x.x' into helper_energy

7764007

Signed-off-by: flora-hofmann-frequenz <[email protected]>

cwasicki reviewed Sep 10, 2024

View reviewed changes

flora-hofmann-frequenz mentioned this pull request Sep 12, 2024

Add helper function to calculate energy metric frequenz-floss/frequenz-reporting-python#7

Merged

flora-hofmann-frequenz closed this Sep 12, 2024



		# pylint: disable=too-many-arguments
		async def cumulative_energy(

Uh oh!

Add helper function to calculate energy metric #93

Add helper function to calculate energy metric #93

Uh oh!

Conversation

flora-hofmann-frequenz commented Sep 3, 2024

Uh oh!

flora-hofmann-frequenz commented Sep 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

noah-kreutzer-frequenz commented Sep 3, 2024

Uh oh!

noah-kreutzer-frequenz commented Sep 3, 2024

Uh oh!

flora-hofmann-frequenz commented Sep 3, 2024

Uh oh!

noah-kreutzer-frequenz commented Sep 3, 2024

Uh oh!

noah-kreutzer-frequenz commented Sep 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flora-hofmann-frequenz commented Sep 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flora-hofmann-frequenz commented Sep 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants