Carry-over-state: plot.py was not updated. #14
Conversation
…ng to plot arrays of different lengths. Now also uses env.metrics.episode_*
📝 WalkthroughWalkthroughThe pull request renames public metric attributes in the plotting module to use an "episode_" prefix throughout calculations, titles, and display fields. This affects 19 distinct metrics across plot computations and derived field calculations, including minor adjustments to Y-axis limits and division-by-zero guards. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/plot.py`:
- Around line 68-77: The title formatting performs unguarded divisions using
env.metrics.episode_baseline_cost and env.metrics.episode_baseline_cost_off
inside the plt.title f-string (lines constructing percentage values), which can
raise ZeroDivisionError; precompute the two percentage values (e.g.,
baseline_savings_pct and baseline_off_savings_pct) before calling plt.title
using the same guard pattern as completion_rate/baseline_completion_rate (check
if baseline value > 0 and otherwise set 0 or "0.0"), then reference those safe
variables in the f-string so the divisions are never done inline in plt.title.
| plt.title(f"{env.session} | ep:{env.current_episode} step:{env.current_step} | {env.weights}\n" | ||
| f"Cost: €{env.metrics.total_cost:.0f}, Base: €{env.metrics.baseline_cost:.0f} " | ||
| f"(+{env.metrics.baseline_cost - env.metrics.total_cost:.0f}, {((env.metrics.baseline_cost - env.metrics.total_cost) / env.metrics.baseline_cost) * 100:.1f}%), " | ||
| f"Base_Off: €{env.metrics.baseline_cost_off:.0f} " | ||
| f"(+{env.metrics.baseline_cost_off - env.metrics.total_cost:.0f}, {((env.metrics.baseline_cost_off - env.metrics.total_cost) / env.metrics.baseline_cost_off) * 100:.1f}%)\n" | ||
| f"Jobs: {env.metrics.jobs_completed}/{env.metrics.jobs_submitted} ({completion_rate:.0f}%, " | ||
| f"wait={avg_wait:.1f}h, Q={env.metrics.max_queue_size_reached}) | " | ||
| f"Base: {env.metrics.baseline_jobs_completed}/{env.metrics.baseline_jobs_submitted} ({baseline_completion_rate:.0f}%, " | ||
| f"wait={baseline_avg_wait:.1f}h, Q={env.metrics.baseline_max_queue_size_reached})", | ||
| f"Cost: €{env.metrics.episode_total_cost:.0f}, Base: €{env.metrics.episode_baseline_cost:.0f} " | ||
| f"(+{env.metrics.episode_baseline_cost - env.metrics.episode_total_cost:.0f}, {((env.metrics.episode_baseline_cost - env.metrics.episode_total_cost) / env.metrics.episode_baseline_cost) * 100:.1f}%), " | ||
| f"Base_Off: €{env.metrics.episode_baseline_cost_off:.0f} " | ||
| f"(+{env.metrics.episode_baseline_cost_off - env.metrics.episode_total_cost:.0f}, {((env.metrics.episode_baseline_cost_off - env.metrics.episode_total_cost) / env.metrics.episode_baseline_cost_off) * 100:.1f}%)\n" | ||
| f"Jobs: {env.metrics.episode_jobs_completed}/{env.metrics.episode_jobs_submitted} ({completion_rate:.0f}%, " | ||
| f"wait={avg_wait:.1f}h, Q={env.metrics.episode_max_queue_size_reached}) | " | ||
| f"Base: {env.metrics.episode_baseline_jobs_completed}/{env.metrics.episode_baseline_jobs_submitted} ({baseline_completion_rate:.0f}%, " | ||
| f"wait={baseline_avg_wait:.1f}h, Q={env.metrics.episode_baseline_max_queue_size_reached})", | ||
| fontsize=9) |
There was a problem hiding this comment.
Potential division by zero in percentage calculations.
Lines 70 and 72 perform division by episode_baseline_cost and episode_baseline_cost_off respectively without guards, unlike the well-protected calculations on lines 47-66. If either baseline cost is zero, this will raise a ZeroDivisionError.
🛠️ Suggested fix using inline conditionals
plt.title(f"{env.session} | ep:{env.current_episode} step:{env.current_step} | {env.weights}\n"
- f"Cost: €{env.metrics.episode_total_cost:.0f}, Base: €{env.metrics.episode_baseline_cost:.0f} "
- f"(+{env.metrics.episode_baseline_cost - env.metrics.episode_total_cost:.0f}, {((env.metrics.episode_baseline_cost - env.metrics.episode_total_cost) / env.metrics.episode_baseline_cost) * 100:.1f}%), "
- f"Base_Off: €{env.metrics.episode_baseline_cost_off:.0f} "
- f"(+{env.metrics.episode_baseline_cost_off - env.metrics.episode_total_cost:.0f}, {((env.metrics.episode_baseline_cost_off - env.metrics.episode_total_cost) / env.metrics.episode_baseline_cost_off) * 100:.1f}%)\n"
+ f"Cost: €{env.metrics.episode_total_cost:.0f}, Base: €{env.metrics.episode_baseline_cost:.0f} "
+ f"(+{env.metrics.episode_baseline_cost - env.metrics.episode_total_cost:.0f}, "
+ f"{((env.metrics.episode_baseline_cost - env.metrics.episode_total_cost) / env.metrics.episode_baseline_cost * 100) if env.metrics.episode_baseline_cost > 0 else 0:.1f}%), "
+ f"Base_Off: €{env.metrics.episode_baseline_cost_off:.0f} "
+ f"(+{env.metrics.episode_baseline_cost_off - env.metrics.episode_total_cost:.0f}, "
+ f"{((env.metrics.episode_baseline_cost_off - env.metrics.episode_total_cost) / env.metrics.episode_baseline_cost_off * 100) if env.metrics.episode_baseline_cost_off > 0 else 0:.1f}%)\n"Alternatively, pre-compute these percentages with guards (similar to completion_rate on lines 47-51) for cleaner formatting.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| plt.title(f"{env.session} | ep:{env.current_episode} step:{env.current_step} | {env.weights}\n" | |
| f"Cost: €{env.metrics.total_cost:.0f}, Base: €{env.metrics.baseline_cost:.0f} " | |
| f"(+{env.metrics.baseline_cost - env.metrics.total_cost:.0f}, {((env.metrics.baseline_cost - env.metrics.total_cost) / env.metrics.baseline_cost) * 100:.1f}%), " | |
| f"Base_Off: €{env.metrics.baseline_cost_off:.0f} " | |
| f"(+{env.metrics.baseline_cost_off - env.metrics.total_cost:.0f}, {((env.metrics.baseline_cost_off - env.metrics.total_cost) / env.metrics.baseline_cost_off) * 100:.1f}%)\n" | |
| f"Jobs: {env.metrics.jobs_completed}/{env.metrics.jobs_submitted} ({completion_rate:.0f}%, " | |
| f"wait={avg_wait:.1f}h, Q={env.metrics.max_queue_size_reached}) | " | |
| f"Base: {env.metrics.baseline_jobs_completed}/{env.metrics.baseline_jobs_submitted} ({baseline_completion_rate:.0f}%, " | |
| f"wait={baseline_avg_wait:.1f}h, Q={env.metrics.baseline_max_queue_size_reached})", | |
| f"Cost: €{env.metrics.episode_total_cost:.0f}, Base: €{env.metrics.episode_baseline_cost:.0f} " | |
| f"(+{env.metrics.episode_baseline_cost - env.metrics.episode_total_cost:.0f}, {((env.metrics.episode_baseline_cost - env.metrics.episode_total_cost) / env.metrics.episode_baseline_cost) * 100:.1f}%), " | |
| f"Base_Off: €{env.metrics.episode_baseline_cost_off:.0f} " | |
| f"(+{env.metrics.episode_baseline_cost_off - env.metrics.episode_total_cost:.0f}, {((env.metrics.episode_baseline_cost_off - env.metrics.episode_total_cost) / env.metrics.episode_baseline_cost_off) * 100:.1f}%)\n" | |
| f"Jobs: {env.metrics.episode_jobs_completed}/{env.metrics.episode_jobs_submitted} ({completion_rate:.0f}%, " | |
| f"wait={avg_wait:.1f}h, Q={env.metrics.episode_max_queue_size_reached}) | " | |
| f"Base: {env.metrics.episode_baseline_jobs_completed}/{env.metrics.episode_baseline_jobs_submitted} ({baseline_completion_rate:.0f}%, " | |
| f"wait={baseline_avg_wait:.1f}h, Q={env.metrics.episode_baseline_max_queue_size_reached})", | |
| fontsize=9) | |
| plt.title(f"{env.session} | ep:{env.current_episode} step:{env.current_step} | {env.weights}\n" | |
| f"Cost: €{env.metrics.episode_total_cost:.0f}, Base: €{env.metrics.episode_baseline_cost:.0f} " | |
| f"(+{env.metrics.episode_baseline_cost - env.metrics.episode_total_cost:.0f}, " | |
| f"{((env.metrics.episode_baseline_cost - env.metrics.episode_total_cost) / env.metrics.episode_baseline_cost * 100) if env.metrics.episode_baseline_cost > 0 else 0:.1f}%), " | |
| f"Base_Off: €{env.metrics.episode_baseline_cost_off:.0f} " | |
| f"(+{env.metrics.episode_baseline_cost_off - env.metrics.episode_total_cost:.0f}, " | |
| f"{((env.metrics.episode_baseline_cost_off - env.metrics.episode_total_cost) / env.metrics.episode_baseline_cost_off * 100) if env.metrics.episode_baseline_cost_off > 0 else 0:.1f}%)\n" | |
| f"Jobs: {env.metrics.episode_jobs_completed}/{env.metrics.episode_jobs_submitted} ({completion_rate:.0f}%, " | |
| f"wait={avg_wait:.1f}h, Q={env.metrics.episode_max_queue_size_reached}) | " | |
| f"Base: {env.metrics.episode_baseline_jobs_completed}/{env.metrics.episode_baseline_jobs_submitted} ({baseline_completion_rate:.0f}%, " | |
| f"wait={baseline_avg_wait:.1f}h, Q={env.metrics.episode_baseline_max_queue_size_reached})", | |
| fontsize=9) |
🤖 Prompt for AI Agents
In `@src/plot.py` around lines 68 - 77, The title formatting performs unguarded
divisions using env.metrics.episode_baseline_cost and
env.metrics.episode_baseline_cost_off inside the plt.title f-string (lines
constructing percentage values), which can raise ZeroDivisionError; precompute
the two percentage values (e.g., baseline_savings_pct and
baseline_off_savings_pct) before calling plt.title using the same guard pattern
as completion_rate/baseline_completion_rate (check if baseline value > 0 and
otherwise set 0 or "0.0"), then reference those safe variables in the f-string
so the divisions are never done inline in plt.title.
Plotting function was trying to plot arrays of different lengths. Now also uses env.metrics.episode_*