Conversation
There was a problem hiding this comment.
Pull request overview
Updates PredBat’s prediction model and test suite to better represent and assert PV “clipping” behavior, particularly around export limits, as discussed in issue #3828.
Changes:
- Adjusts clipping accounting in the prediction engine when applying inverter/export limits.
- Extends the test harness (
simple_scenario) with anassert_clippedparameter and validates predicted clipped energy. - Adds/updates multiple model scenarios to assert clipped-energy totals under different PV/battery/export-limit conditions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/predbat/prediction.py | Updates how clipped energy is accumulated when inverter limit / export limit logic reduces PV output. |
| apps/predbat/tests/test_infra.py | Adds assert_clipped to simple_scenario and checks the predicted clipped total. |
| apps/predbat/tests/test_model.py | Updates many scenarios to assert clipped totals and adds an export-limit/no-solar regression scenario. |
| pv_ac_no_loss = max(pv_ac_before - over_limit, 0) | ||
| clipped_today += pv_ac_before - pv_ac_no_loss |
There was a problem hiding this comment.
In the hybrid inverter-limit clipping path, over_limit is in the same units as total_inverted (it includes PV as pv_ac / inverter_loss). The current clipped_today calculation mixes units by subtracting over_limit directly from pv_ac_before (AC energy), which will over/under-count clipping when inverter_loss != 1. Consider calculating clipped energy consistently from the actual PV reduction (e.g., based on pv_ac_before - pv_ac), or otherwise convert over_limit into the same units as pv_ac before using it.
| pv_ac_no_loss = max(pv_ac_before - over_limit, 0) | |
| clipped_today += pv_ac_before - pv_ac_no_loss | |
| clipped_today += pv_ac_before - pv_ac |
| total_clipped = max(prediction.predict_clipped_best.values()) if prediction.predict_clipped_best else 0 | ||
| if total_clipped > 0: | ||
| print("ERROR: export_limit_no_clip_no_solar: clipping should be 0 but got {}".format(total_clipped)) | ||
| failed = True |
There was a problem hiding this comment.
This scenario manually inspects prediction.predict_clipped_best and prints an error if any clipping occurred. Since simple_scenario now supports assert_clipped, this can be simplified by passing assert_clipped=0 and letting simple_scenario handle the assertion (and you can drop return_prediction_handle=True unless it's needed elsewhere). This reduces duplication and keeps assertion logic in one place.
#3828