-
-
Notifications
You must be signed in to change notification settings - Fork 90
Fix issue with plan and 1 minutes #2415
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 updates the version, refines plan rendering logic, and adjusts the charge window finder loop.
- Bump
THIS_VERSIONfromv8.20.6tov8.20.7 - Use upward rounding and bounding for
periods_leftandrowspanin HTML plan output - Clarify manual‐times condition and change loop increment from 1 to 5 minutes in
find_charge_window
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/predbat/predbat.py | Version bump to v8.20.7 |
| apps/predbat/output.py | Adjust rounding for periods_left and cap rowspan to avoid overflow |
| apps/predbat/fetch.py | Parenthesized manual‐times checks and increased minute increment |
Comments suppressed due to low confidence (1)
apps/predbat/fetch.py:1372
- Incrementing
minuteby 5 skips intermediate minutes, potentially missing valid charge windows. Verify that the coarser 5-minute resolution is intended or revert to a 1-minute step.
minute += 5
| export_window_n = -1 | ||
| in_alert = True if self.alert_active_keep.get(minute, 0) > 0 else False | ||
| periods_left = int((end_plan - minute) / 30) | ||
| periods_left = int((end_plan - minute + 29) / 30) |
Copilot
AI
Jun 1, 2025
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.
[nitpick] Consider using math.ceil((end_plan - minute) / 30) for clarity instead of manually adding 29; this makes the intention to round up explicit.
| periods_left = int((end_plan - minute + 29) / 30) | |
| periods_left = math.ceil((end_plan - minute) / 30) |
| if export_window_n >= 0 and not in_span: | ||
| export_end_minute = self.export_window_best[export_window_n]["end"] | ||
| rowspan = int((export_end_minute - minute) / 30) | ||
| rowspan = min(int((export_end_minute - minute) / 30), periods_left) |
Copilot
AI
Jun 1, 2025
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.
[nitpick] The floor division here may lead to off-by-one for partial slots; consider whether you need a ceiling or add a comment to explain why floor plus a min cap is correct.
| rowspan = min(int((export_end_minute - minute) / 30), periods_left) | |
| rowspan = min(math.ceil((export_end_minute - minute) / 30), periods_left) |
| if minute in rates: | ||
| rate = rates[minute] | ||
| if ((not find_high) and (rate <= threshold_rate)) or (find_high and (rate >= threshold_rate) and (rate > 0)) or minute in self.manual_all_times or rate_low_start in self.manual_all_times: | ||
| if ((not find_high) and (rate <= threshold_rate)) or (find_high and (rate >= threshold_rate) and (rate > 0)) or (minute in self.manual_all_times) or (rate_low_start in self.manual_all_times): |
Copilot
AI
Jun 1, 2025
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.
[nitpick] This combined condition is lengthy and a bit hard to parse. Extracting sub-conditions into well-named booleans (e.g., is_manual_time) would improve readability.
| if ((not find_high) and (rate <= threshold_rate)) or (find_high and (rate >= threshold_rate) and (rate > 0)) or (minute in self.manual_all_times) or (rate_low_start in self.manual_all_times): | |
| is_rate_below_threshold = (not find_high) and (rate <= threshold_rate) | |
| is_rate_above_threshold = find_high and (rate >= threshold_rate) and (rate > 0) | |
| is_manual_time = (minute in self.manual_all_times) or (rate_low_start in self.manual_all_times) | |
| if is_rate_below_threshold or is_rate_above_threshold or is_manual_time: |
#2411