feat: add a column mapper class #164

Mohammad-Tayyab-Frequenz · 2025-09-24T10:48:14Z

No description provided.

cwasicki

Just from this PR it's not clear what this is about because it is missing PR and commit descriptions and the doc is not sufficient. This is missing a motivation why this class is needed (in the future) and how it is intended to be used.

I am also not sure about a couple of name matchings. This stresses the point that we need well-defined metric definitions. In fact I think we should add the module with the definitions of all of these metrics used here either before this PR or add them here.

src/frequenz/lib/notebooks/reporting/schema_mapping.yaml

cwasicki · 2025-09-26T07:23:09Z

src/frequenz/lib/notebooks/reporting/utils/column_mapper.py

+"""Column mapping utilities for energy reporting.
+
+Provides the `ColumnMapper` dataclass to manage renaming between raw,
+canonical, and localized display column names. Supports loading schema


What are raw, canonical and display names? Where are these used?

I will update this in the documentation.

Raw - directly from reporting api
Canonical - used in our codebase
Display - To be displayed in the notebooks

src/frequenz/lib/notebooks/reporting/utils/column_mapper.py

src/frequenz/lib/notebooks/reporting/schema_mapping.yaml

cyiallou · 2025-09-26T09:19:24Z

When I added EN/DE support for the solar notebook, I faced a similar challenge. At the time I went for a simple TranslationManager class with an internal dictionary of text + translations (link). It’s admittedly not the most elegant solution, but it avoids introducing an extra dependency in this repo (which is itself consumed elsewhere). That trade-off was one of the reasons I kept it simple.

I see the YAML approach here is more structured (which is definitely a plus for maintainability). Just wanted to flag the existing translator.py so you can consider whether it makes sense to adapt/reuse it, or whether your case really needs a separate schema + class.

Mohammad-Tayyab-Frequenz · 2025-10-01T11:27:20Z

The translator.py script looks well-structured and offers a wide range of functionalities. My current solution is more narrowly focused, handling only the translation of column display names into the required language. It might be worthwhile to combine both approaches and define a comprehensive configuration within a .yaml file. Do you think this would be a feasible path forward?

Updates the requirements on [kaleido](https://github.com/plotly/kaleido) to permit the latest version. - [Release notes](https://github.com/plotly/kaleido/releases) - [Commits](plotly/Kaleido@v0.2.1...v1.1.0) --- updated-dependencies: - dependency-name: kaleido dependency-version: 1.1.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]>

Mohammad-Tayyab-Frequenz · 2025-10-02T11:01:36Z

src/frequenz/lib/notebooks/reporting/utils/helpers.py

+    df_with_pv_flows["pv_self"] = (
+        df_with_pv_flows["pv_prod"] - df_with_pv_flows["pv_excess"]
+    ).clip(lower=0)
+


I am not sure if calculating pv_self as shown here the correct way of doing this. I have picked this directly from the previous reporting NBs.

src/frequenz/lib/notebooks/reporting/schema_mapping.yaml

Signed-off-by: Mohammad Tayyab <[email protected]>

Copilot

Pull Request Overview

This PR adds a column mapping utility for energy reporting notebooks to insulate them from upstream schema changes. The implementation provides locale-aware column name translations and timezone handling through a YAML configuration file.

Key changes:

New ColumnMapper class that translates between raw API headers, canonical names, and localized display labels
YAML schema configuration file defining column mappings, descriptions, and locale-specific labels
Helper functions for PV energy flow calculations

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`src/frequenz/lib/notebooks/reporting/utils/column_mapper.py`	Core ColumnMapper class with YAML loading and locale-aware column translation
`src/frequenz/lib/notebooks/reporting/schema_mapping.yaml`	Schema configuration defining column mappings and display labels
`src/frequenz/lib/notebooks/reporting/utils/helpers.py`	PV energy flow calculation utilities
`src/frequenz/lib/notebooks/reporting/utils/__init__.py`	Package initialization file
`pyproject.toml`	Added PyYAML dependencies and updated kaleido version
`RELEASE_NOTES.md`	Documentation of the new ColumnMapper feature

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

src/frequenz/lib/notebooks/reporting/utils/helpers.py

src/frequenz/lib/notebooks/reporting/utils/column_mapper.py

Signed-off-by: Mohammad Tayyab <[email protected]>

src/frequenz/lib/notebooks/reporting/utils/column_mapper.py

cwasicki · 2025-10-08T11:01:58Z

src/frequenz/lib/notebooks/reporting/utils/helpers.py

+    Args:
+      df: Input DataFrame. If present, uses columns ``pv_neg``,
+        ``consumption``, and ``battery_pos``. Missing columns are
+        treated as zeros.


I would raise if any of the expected columns is missing, otherwise it's easy to create typo bugs.

Yeah, I picked it from the notebooks. I think previously sometimes consumption column would be missing so this logic was implemented but now we should raise if any column name is missing.
But one question would be whether pv_columns and battery columns will be present in the data even if the component is missing?

So, I think I need to add the logic to check whether the component type is present in the data. Then forward with the checks/calculations otherwise pass.

cwasicki · 2025-10-08T11:02:52Z

src/frequenz/lib/notebooks/reporting/utils/helpers.py

+      Newly created/updated columns:
+        - ``pv_prod``: PV production as a positive series (negated/clipped from
+          ``pv_neg``).
+        - ``pv_excess``: Excess PV after subtracting household consumption.


positive or negative?

So, currently the logic is to multiply pv_neg * -1 to get the pv prod in positive float.

cwasicki · 2025-10-08T11:42:18Z

src/frequenz/lib/notebooks/reporting/utils/helpers.py

+import pandas as pd
+
+
+def _add_pv_energy_flows(df: pd.DataFrame) -> pd.DataFrame:


This function is problematic if there is more than one source for production, e.g. from CHP. Options to deal with this could be to

Make this to work not with PV but "production" in general.

Enforce that PV is the only source and otherwise raise. Then we cannot use it on certain (already existing) microgrids though, so this can only be a temporary solution.

Take into account all sources. This requires some accounting logic though, which can be case-dependent.

Mohammad-Tayyab-Frequenz requested review from cwasicki and cyiallou September 24, 2025 10:48

Mohammad-Tayyab-Frequenz self-assigned this Sep 24, 2025

Mohammad-Tayyab-Frequenz requested a review from a team as a code owner September 24, 2025 10:48

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

Mohammad-Tayyab-Frequenz force-pushed the add-schema-mapping branch 2 times, most recently from 4afd9b8 to de48ef9 Compare September 24, 2025 13:17

cwasicki requested changes Sep 26, 2025

View reviewed changes

Mohammad-Tayyab-Frequenz force-pushed the add-schema-mapping branch from de48ef9 to 8e0cd67 Compare October 2, 2025 09:38

Mohammad-Tayyab-Frequenz force-pushed the add-schema-mapping branch 2 times, most recently from e659ea3 to dae7d8c Compare October 2, 2025 10:08

Mohammad-Tayyab-Frequenz commented Oct 2, 2025

View reviewed changes

Mohammad-Tayyab-Frequenz commented Oct 6, 2025

View reviewed changes

src/frequenz/lib/notebooks/reporting/schema_mapping.yaml Outdated Show resolved Hide resolved

Mohammad-Tayyab-Frequenz commented Oct 6, 2025

View reviewed changes

src/frequenz/lib/notebooks/reporting/schema_mapping.yaml Outdated Show resolved Hide resolved

Mohammad-Tayyab-Frequenz commented Oct 6, 2025

View reviewed changes

src/frequenz/lib/notebooks/reporting/schema_mapping.yaml Show resolved Hide resolved

Mohammad-Tayyab-Frequenz force-pushed the add-schema-mapping branch from dae7d8c to 8cbfef0 Compare October 7, 2025 10:09

feat: add a column mapper class

4487378

Signed-off-by: Mohammad Tayyab <[email protected]>

Mohammad-Tayyab-Frequenz requested a review from Copilot October 7, 2025 11:22

Copilot AI reviewed Oct 7, 2025

View reviewed changes

src/frequenz/lib/notebooks/reporting/utils/helpers.py Outdated Show resolved Hide resolved

src/frequenz/lib/notebooks/reporting/utils/helpers.py Outdated Show resolved Hide resolved

src/frequenz/lib/notebooks/reporting/utils/column_mapper.py Show resolved Hide resolved

Mohammad-Tayyab-Frequenz added 2 commits October 7, 2025 13:37

feat: add pv metrics calculation function

3d5acbe

Signed-off-by: Mohammad Tayyab <[email protected]>

docs: update release notes.md

189099b

Signed-off-by: Mohammad Tayyab <[email protected]>

Mohammad-Tayyab-Frequenz force-pushed the add-schema-mapping branch from 8cbfef0 to 189099b Compare October 7, 2025 11:37

Mohammad-Tayyab-Frequenz requested a review from cwasicki October 7, 2025 11:39

cwasicki reviewed Oct 8, 2025

View reviewed changes

		import pandas as pd


		def _add_pv_energy_flows(df: pd.DataFrame) -> pd.DataFrame:

feat: add a column mapper class #164

Are you sure you want to change the base?

feat: add a column mapper class #164

Uh oh!

Conversation

Mohammad-Tayyab-Frequenz commented Sep 24, 2025

Uh oh!

cwasicki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cyiallou commented Sep 26, 2025

Uh oh!

Mohammad-Tayyab-Frequenz commented Oct 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mohammad-Tayyab-Frequenz Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Mohammad-Tayyab-Frequenz Oct 8, 2025 •

edited

Loading