Remove prod positive argument#275
Remove prod positive argument#275Mohammad-Tayyab-Frequenz wants to merge 2 commits intofrequenz-floss:v0.x.xfrom
Conversation
Signed-off-by: Mohammad Tayyab <Mohammad.Tayyab@neustrom.de>
Signed-off-by: Mohammad Tayyab <Mohammad.Tayyab@neustrom.de>
github-actions
bot
added
part:docs
There was a problem hiding this comment.
Pull request overview
This PR removes the production_is_positive toggle and standardizes reporting metrics to follow PSC sign conventions, where production inputs are expected to be negative and are sign-corrected internally.
Changes:
- Removed
production_is_positivefrom reporting metrics helpers andadd_energy_flows(). - Updated
add_energy_flows()to convert internally computed positive production totals back to PSC-negative before calling metrics helpers. - Updated tests and release notes to reflect the new fixed sign convention.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/frequenz/lib/notebooks/reporting/metrics/reporting_metrics.py |
Removes the toggle from helper APIs and makes production sign handling unconditional (PSC). |
src/frequenz/lib/notebooks/reporting/utils/helpers.py |
Drops the toggle from add_energy_flows() and updates downstream helper calls to pass PSC-negative production. |
src/frequenz/lib/notebooks/reporting/data_processing.py |
Updates add_energy_flows() call site for the new signature. |
tests/test_reporting_metrics.py |
Updates the production sign convention in the asset production test (needs further consistency fixes). |
RELEASE_NOTES.md |
Documents the breaking change and upgrade guidance. |
Comments suppressed due to low confidence (1)
src/frequenz/lib/notebooks/reporting/utils/helpers.py:445
- The comment says production is already positive, but the call converts
production_totalback to PSC-negative via* -1before passing intoproduction_excess(). Please reword this comment to reflect what the code is doing (e.g., production_total is positive afterasset_production(), then converted back to PSC convention for downstream metrics helpers).
# Surplus vs. consumption (production is already positive because of the above cleaning)
df_flows["production_excess"] = production_excess(
df_flows["production_total"] * -1,
df_flows["consumption_total"],
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def production_excess(production: pd.Series, consumption: pd.Series) -> pd.Series: | ||
| """Compute the excess production relative to consumption. | ||
|
|
||
| Calculates surplus by subtracting consumption from production and removing | ||
| negative results. Production is optionally sign-corrected first. | ||
|
|
||
| Args: | ||
| production: Series of production values (e.g., kW or MW). | ||
| production: Series of production values (PSC convention, negative). | ||
| consumption: Series of consumption values (same units as `production`). | ||
| production_is_positive: Whether production values are already positive. | ||
| If False, `production` is inverted before clipping. | ||
|
|
There was a problem hiding this comment.
production_excess() docstring still says production is “optionally sign-corrected first”, but the function now always assumes PSC convention and unconditionally sign-corrects via asset_production(production). Please update the docstring to match the new behavior (remove “optionally” wording and clarify the expected PSC-negative input).
| Args: | ||
| production: Series of production values (e.g., kW or MW). | ||
| consumption: Series of consumption values (same units as `production`). | ||
| production_is_positive: Whether production values are already positive. | ||
| If False, `production` is inverted before clipping. | ||
|
|
There was a problem hiding this comment.
production_self_consumption() now assumes PSC convention (production negative), but the Args section still describes production generically (“production values”), which can be misread as already-positive input. Please update the parameter docs here (and consistently for related self-* helpers if needed) to explicitly state PSC-negative production is expected.
| def test_asset_production_respects_sign_and_missing_values() -> None: | ||
| """Positive flag controls sign inversion and NaNs stay missing.""" | ||
| production = pd.Series([10, -5, None], index=pd.RangeIndex(3)) | ||
| """Production is negated and NaNs stay missing.""" | ||
| production = pd.Series([-10, 5, None], index=pd.RangeIndex(3)) | ||
|
|
||
| positive = metrics.asset_production(production, production_is_positive=True) | ||
| expected_positive = pd.Series([10.0, 0.0, float("nan")], index=production.index) | ||
| assert_series_equal(positive, expected_positive) | ||
|
|
||
| negative = metrics.asset_production(production, production_is_positive=False) | ||
| expected_negative = pd.Series([0.0, 5.0, float("nan")], index=production.index) | ||
| assert_series_equal(negative, expected_negative) | ||
| result = metrics.asset_production(production) | ||
| expected = pd.Series([10.0, 0.0, float("nan")], index=production.index) | ||
| assert_series_equal(result, expected) |
There was a problem hiding this comment.
This PR standardizes PSC sign conventions (production negative), but there is still at least one test in this file that uses a positive production input for grid_consumption() (later in the file). Please update remaining tests to consistently pass PSC-negative production so they exercise the intended convention and don’t lock in the old behavior.
Remove the production_is_positive toggle and standardize reporting metrics to always use PSC sign conventions (production is negative). Update all call sites and tests to pass -production where needed, and document the change in release notes.