-
Notifications
You must be signed in to change notification settings - Fork 16
Enable BatteryPriceSOCController to handle multiple battery durations [+bugfix for high/low SOC]
#85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances the BatteryPriceSOCController to support batteries with variable durations (up to 12 hours) rather than being hardcoded for 4-hour batteries. It fixes a critical bug in the SOC threshold logic and improves documentation.
Key Changes:
- Fixed bug where discharge/charge logic used incorrect SOC comparisons (lines 248, 252 in controller)
- Added dynamic duration calculation based on energy_capacity/power_capacity ratio
- Enhanced class-level docstring with comprehensive algorithm explanation
- Updated existing tests and added new 2-hour battery test case
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| hycon/controllers/battery_controller.py | Added duration calculation, validation, and comprehensive docstring; fixed SOC comparison logic in compute_controls |
| tests/battery_test.py | Updated test assertions to match corrected logic; added new test for 2-hour battery configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
misi9170
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks good to me. Some of the Copilot comments are probably worth addressing, and I've weighed in on a couple. Feel free to re-request my review when you're happy and I'll go through once more to approve.
incoming... think I was on your heels going back through comments |
BatteryPriceSOCController to handle multiple battery durations [+bugfix for high/low SOC]
This PR accomplishes 3 things:
BatteryPriceSOCControlleris corrected and tests are updated to passBatteryPriceSOCControlleris updated to allow for non-4 hour battery durations (up to 12). A new test is added to test a non-4 hour battery configuration as well.BatteryPriceSOCControlleris made a bit more descriptive for future reference (contrast with similar controllers)