Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…tion
- Add tariff_zone_1/2 and zone boundaries as constructor parameters
instead of post-init attribute assignment
- Implement get_raw_data_from_provider() returning {} (no external API)
- Add _validate_price() static method for positive-float enforcement
- Add properties with validation for tariff_zone_1 and tariff_zone_2
- Guard _get_prices_native() with RuntimeError if prices not set
- Log warning when zone_1_start == zone_1_end (0 hours coverage)
- Fix wrap-around logic: use or-condition instead of negation
- Fix bare except Exception -> (ValueError, TypeError)
- Fix range(0, 48) -> range(48)
- Fix field not in config.keys() -> field not in config
- Remove extra blank line in __init__
Tests:
- Remove DummyTariffZones subclass (TariffZones now concrete)
- Add _validate_price tests
- Add constructor/defaults tests
- Add test for prices-unset RuntimeError
- Add wrap-around schedule test
- Add equal start/end warning test
- Add DynamicTariff factory integration tests
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace zone_1_start/zone_1_end config parameters with explicit hour-list assignment per zone: Config parameters: - zone_1_hours: comma-separated hours for zone 1 (e.g. 7,8,9,...,22) - zone_2_hours: comma-separated hours for zone 2 (e.g. 0,1,...,6,23) - zone_3_hours: optional third zone hours - tariff_zone_3: price for optional third zone Validation rules (all enforced at price-generation time): - No hour may appear in more than one zone (ValueError) - No duplicate within a single zone (ValueError) - All 24 hours 0-23 must be covered (ValueError) - zone_3_hours and tariff_zone_3 must both be set or both omitted Input formats accepted for *_hours: comma-separated string (as YAML provides), Python list/tuple, or single integer. Updated: - TariffZones: new _parse_hours(), _validate_configuration(), properties - DynamicTariff factory: requires zone_1_hours + zone_2_hours - batcontrol_config_dummy.yaml: updated example comments - Tests: 24 tests covering parse, validation, 2-zone, 3-zone, factory Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
_parse_hours now accepts inclusive range tokens (e.g. '0-5') in addition to single values, enabling compact config like: zone_1_hours: 7-22 zone_2_hours: 0-6,23 zone_3_hours: 17-20 Ranges are expanded inclusively (7-22 → [7,8,...,22]). Single integers and range strings may be freely mixed. List/tuple elements that are Python ints are handled directly (no string conversion) to avoid treating negative integers as ranges. Validation unchanged: inverted ranges (5-3) and out-of-bounds values raise ValueError. Updated config dummy example to use range notation. Added 7 new parse_hours tests covering ranges. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new static dynamic-tariff provider that supports up to three fixed “tariff zones” (hour lists) so batcontrol can work with zone-based pricing (e.g., peak/off-peak and an optional third zone such as heat-pump tariffs).
Changes:
- Introduces
TariffZonesprovider that maps hour-of-day (0–23) to fixed prices across up to three zones. - Extends
DynamicTariff.create_tarif_providerto instantiate the newtariff_zonesprovider. - Adds configuration/documentation updates plus a dedicated pytest suite and a Windows
run_tests.ps1helper.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/batcontrol/dynamictariff/tariffzones.py |
New provider implementing up to 3 static tariff zones and hour parsing/validation. |
src/batcontrol/dynamictariff/dynamictariff.py |
Factory updated to support type: tariff_zones. |
tests/batcontrol/dynamictariff/test_tariffzones.py |
New tests covering hour parsing, validation, 2-zone/3-zone mapping, and factory integration. |
config/batcontrol_config_dummy.yaml |
Documents new tariff_zones configuration keys. |
README.MD |
Updates prerequisites to mention zone-based pricing. |
run_tests.ps1 |
Adds a PowerShell test runner for Windows. |
| If you prefer a more even distribution during the low price hours, you can adjust the | ||
| soften_price_difference_on_charging to enabled | ||
| and | ||
| max_grid_charge_rate to a low value, e.g. capacity of the battery divided | ||
| by the hours of low price periods. | ||
|
|
||
| If you prefer a late charging start (=optimize efficiency, have battery only short | ||
| time at high SOC), you can adjust the |
There was a problem hiding this comment.
Docstring wording is ungrammatical/unclear: “soften_price_difference_on_charging to enabled/disabled”. Consider rephrasing to “set soften_price_difference_on_charging to enabled/disabled” (and similarly for max_grid_charge_rate) so users can follow the configuration guidance.
| If you prefer a more even distribution during the low price hours, you can adjust the | |
| soften_price_difference_on_charging to enabled | |
| and | |
| max_grid_charge_rate to a low value, e.g. capacity of the battery divided | |
| by the hours of low price periods. | |
| If you prefer a late charging start (=optimize efficiency, have battery only short | |
| time at high SOC), you can adjust the | |
| If you prefer a more even distribution during the low price hours, you can set | |
| soften_price_difference_on_charging to enabled | |
| and set | |
| max_grid_charge_rate to a low value, e.g. capacity of the battery divided | |
| by the hours of low price periods. | |
| If you prefer a late charging start (=optimize efficiency, have battery only short | |
| time at high SOC), you can set |
| now = datetime.datetime.now().astimezone(t.timezone) | ||
| base = now.replace(minute=0, second=0, microsecond=0) | ||
|
|
||
| for rel_hour, price in prices.items(): | ||
| h = (base + datetime.timedelta(hours=rel_hour)).hour | ||
| expected = t.tariff_zone_1 if h in HOURS_PEAK else t.tariff_zone_2 | ||
| assert price == pytest.approx(expected) |
There was a problem hiding this comment.
This test uses the real current time (datetime.datetime.now()), which makes it non-deterministic and can hide/introduce edge cases around DST changes. Other tariff tests in this repo patch the module’s datetime to a fixed instant; consider doing the same here (and adding an explicit DST-transition regression case for Europe/Berlin) to keep CI stable and validate correct timezone handling.
| now = datetime.datetime.now().astimezone(t.timezone) | ||
| base = now.replace(minute=0, second=0, microsecond=0) | ||
|
|
||
| zone_map = {h: 0.30 for h in peak} | ||
| zone_map.update({h: 0.15 for h in offpeak}) | ||
| zone_map.update({h: 0.22 for h in shoulder}) | ||
|
|
||
| for rel_hour, price in prices.items(): | ||
| h = (base + datetime.timedelta(hours=rel_hour)).hour | ||
| assert price == pytest.approx(zone_map[h]) |
There was a problem hiding this comment.
This test also depends on wall-clock time (datetime.datetime.now()), which reduces determinism and doesn’t validate behavior across DST boundaries. Please patch batcontrol.dynamictariff.tariffzones.datetime to a fixed time like other dynamic tariff tests do, and consider a DST-boundary test where the next 48h crosses a clock change.
| # Activate virtual environment if it exists (Windows path used by venv) | ||
| if (Test-Path -Path .\.venv\Scripts\Activate.ps1) { | ||
| . .\.venv\Scripts\Activate.ps1 |
There was a problem hiding this comment.
The script only activates a virtualenv at ./.venv/..., but run_tests.sh looks for ./venv/activate while the README suggests .venv. This mismatch can lead to tests running outside the intended environment on some setups; consider checking both locations or standardizing on one convention across scripts and docs.
| # Activate virtual environment if it exists (Windows path used by venv) | |
| if (Test-Path -Path .\.venv\Scripts\Activate.ps1) { | |
| . .\.venv\Scripts\Activate.ps1 | |
| # Activate virtual environment if it exists (prefer .venv, fall back to venv) | |
| if (Test-Path -Path .\.venv\Scripts\Activate.ps1) { | |
| . .\.venv\Scripts\Activate.ps1 | |
| } elseif (Test-Path -Path .\venv\Scripts\Activate.ps1) { | |
| . .\venv\Scripts\Activate.ps1 |
|
|
||
| 1. A PV installation with a BYD Battery and a Fronius Gen24 inverter. | ||
| 2. An EPEX Spot based contract with hourly electricity pricing, like Awattar, Tibber etc. (Get a €50 bonus on sign-up to Tibber using this [link](https://invite.tibber.com/x8ci52nj).) | ||
| 2. A zone based pricing, like Octopus, or an EPEX Spot based contract with hourly electricity pricing, like Awattar, Tibber etc. (Get a €50 bonus on sign-up to Tibber using this [link](https://invite.tibber.com/x8ci52nj).) |
There was a problem hiding this comment.
Minor wording: “A zone based pricing” is grammatically awkward; “zone-based pricing” (or “a zone-based tariff”) reads more naturally.
| 2. A zone based pricing, like Octopus, or an EPEX Spot based contract with hourly electricity pricing, like Awattar, Tibber etc. (Get a €50 bonus on sign-up to Tibber using this [link](https://invite.tibber.com/x8ci52nj).) | |
| 2. A zone-based tariff, like Octopus, or an EPEX Spot based contract with hourly electricity pricing, like Awattar, Tibber etc. (Get a €50 bonus on sign-up to Tibber using this [link](https://invite.tibber.com/x8ci52nj).) |
| now = datetime.datetime.now().astimezone(self.timezone) | ||
| current_hour_start = now.replace(minute=0, second=0, microsecond=0) | ||
|
|
||
| prices = {} | ||
| for rel_hour in range(48): | ||
| ts = current_hour_start + datetime.timedelta(hours=rel_hour) | ||
| prices[rel_hour] = hour_to_price[ts.hour] |
There was a problem hiding this comment.
In _get_prices_native, datetime.datetime.now().astimezone(self.timezone) relies on the host local timezone for naive now(), which can misalign indices (especially on Windows where TZ isn’t applied) and can produce incorrect hours around DST changes. Prefer basing calculations on datetime.datetime.now(datetime.timezone.utc) and converting to self.timezone, and avoid adding timedeltas to pytz-localized datetimes (convert each rel_hour from UTC to local instead) so DST transitions map to the correct local hour.
| now = datetime.datetime.now().astimezone(self.timezone) | |
| current_hour_start = now.replace(minute=0, second=0, microsecond=0) | |
| prices = {} | |
| for rel_hour in range(48): | |
| ts = current_hour_start + datetime.timedelta(hours=rel_hour) | |
| prices[rel_hour] = hour_to_price[ts.hour] | |
| # Anchor calculations in UTC and convert to the configured timezone | |
| now_utc = datetime.datetime.now(datetime.timezone.utc) | |
| now_local = now_utc.astimezone(self.timezone) | |
| current_hour_start_local = now_local.replace(minute=0, second=0, microsecond=0) | |
| current_hour_start_utc = current_hour_start_local.astimezone(datetime.timezone.utc) | |
| prices = {} | |
| for rel_hour in range(48): | |
| ts_utc = current_hour_start_utc + datetime.timedelta(hours=rel_hour) | |
| ts_local = ts_utc.astimezone(self.timezone) | |
| prices[rel_hour] = hour_to_price[ts_local.hour] |
| def __init__( | ||
| self, | ||
| timezone, | ||
| min_time_between_API_calls=0, | ||
| delay_evaluation_by_seconds=0, | ||
| target_resolution: int = 60, | ||
| tariff_zone_1: float = None, |
There was a problem hiding this comment.
The __init__ parameter continuation indentation is misaligned with the opening parenthesis and will likely trigger PEP8/pylint continuation-indentation warnings. Please run autopep8 (or align continuation lines under the opening parenthesis / use a 4-space hanging indent) to match the style used in other providers.
Based on the work of @OliJue we are implementing up to three static tarif zones.
Off Olis implementation, we are defining the list of the hours (not start + end).
Also we are introducing the third zone for heatpump tarifs.
Closed #282
Closed #136
Co-Authored by @OliJue