Battery curve support for string keys, unit testing fixes #3126
Conversation
springfall2008
changed the title
There was a problem hiding this comment.
Pull request overview
This PR fixes unit testing issues and adds support for string-keyed battery power curves to improve YAML configuration compatibility. The main changes include refactoring the get_curve_value() function to handle both integer and string dictionary keys, updating find_battery_temperature_cap() to use this helper function, and adding comprehensive tests for string-keyed curves.
Key changes:
- Refactored
get_curve_value()function for better handling of string and integer curve indices - Enhanced
find_battery_temperature_cap()to properly handle string-keyed temperature curves - Added two new unit tests for string-keyed temperature and charge power curves
- Fixed test import naming from
run_optimise_levelstorun_optimise_levels_tests
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/predbat/utils.py | Moved and simplified get_curve_value() function; refactored find_battery_temperature_cap() to use the helper function |
| apps/predbat/tests/test_find_charge_rate.py | Added two new test functions to verify string-keyed curve support for temperature and charge power curves |
| apps/predbat/unit_test.py | Fixed import statement and added new test entries to the test registry |
| apps/predbat/predbat.py | Bumped version to v8.30.9 and added unit_test.py to PREDBAT_FILES list |
| """ | ||
| Get value from curve with integer or string index | ||
| """ | ||
|
|
||
|
|
||
| def get_curve_value(curve, index, default=1.0): |
There was a problem hiding this comment.
The documentation for get_curve_value should use a proper docstring format (triple-quoted string inside the function) instead of a comment above the function. This is inconsistent with the documentation style used throughout the rest of utils.py (e.g., find_battery_temperature_cap, get_charge_rate_curve, calc_percent_limit). The docstring should include Args and Returns sections explaining the parameters and return value.
| """ | |
| Get value from curve with integer or string index | |
| """ | |
| def get_curve_value(curve, index, default=1.0): | |
| def get_curve_value(curve, index, default=1.0): | |
| """ | |
| Get a value from a curve mapping using either an integer or string index. | |
| Args: | |
| curve: Mapping of curve points keyed by index (typically int or str). | |
| index: Index to look up in the curve. Both the raw value and its string | |
| representation will be tried as keys. | |
| default: Value to return if neither the index nor its string form exist | |
| in the curve. | |
| Returns: | |
| The value from the curve corresponding to the given index, or ``default`` | |
| if no matching key is found. | |
| """ |
| Get value from curve with integer or string index | ||
| """ | ||
|
|
||
|
|
||
| def get_curve_value(curve, index, default=1.0): | ||
| if index in curve: | ||
| return curve[index] |
There was a problem hiding this comment.
The get_curve_value function is defined after it is used. Lines 919 and 939 call get_curve_value, but the function is not defined until line 948. This will cause a NameError at runtime when get_charge_rate_curve or get_discharge_rate_curve is called. The function should be defined before these calling functions, not after them.
| if index in curve: | ||
| return curve[index] | ||
| elif str(index) in curve: |
There was a problem hiding this comment.
This instance of list is unhashable.
This instance of list is unhashable.
| if index in curve: | |
| return curve[index] | |
| elif str(index) in curve: | |
| try: | |
| if index in curve: | |
| return curve[index] | |
| except TypeError: | |
| # Unhashable index (e.g. list); fall back to string/default handling | |
| pass | |
| if str(index) in curve: |
No description provided.