Conversation
|
Benchmark comparison for |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several new demonstration notebooks for the Plugboard framework, covering wastewater treatment (BSM1-inspired model) and transport simulations (traffic intersection and bus bunching). The review identified several critical issues where components were missing required initial values for inputs, which would cause the simulations to crash on the first tick. Additionally, feedback was provided on improving the anti-windup logic in the PI controller and addressing a memoryless traffic signal delay implementation in the bus simulation.
| "\n", | ||
| " comps = [\n", | ||
| " SetpointGenerator(name=\"setpoints\"),\n", | ||
| " PIController(name=\"controller\", kp=kp, ki=ki, initial_values={\"h1\": [h1_init], \"h2\": [h2_init]}),\n", |
There was a problem hiding this comment.
The PIController component requires initial values for its sp_h1 and sp_h2 inputs to function correctly in the first tick of the simulation. Without these, self.sp_h1 will be None during the first step() call, leading to a calculation error.
| " PIController(name=\"controller\", kp=kp, ki=ki, initial_values={\"h1\": [h1_init], \"h2\": [h2_init]}),\n", | |
| PIController(name="controller", kp=kp, ki=ki, initial_values={"h1": [h1_init], "h2": [h2_init], "sp_h1": [h1_init], "sp_h2": [h2_init]}), |
| " upper_outlet_area=a3,\n", | ||
| " pump_gain=k1,\n", | ||
| " gamma=gamma1,\n", | ||
| " initial_values={\"prev_h\": [h1_init]},\n", |
There was a problem hiding this comment.
The LowerTank component requires initial values for upper_h and pump_voltage in addition to prev_h. In Plugboard's LocalProcess, inputs for tick 0 are taken from initial_values. If omitted, they default to None, which will cause a crash in the step() method when calculating the square root or pump inflow.
initial_values={"prev_h": [h1_init], "upper_h": [h3_init], "pump_voltage": [V1_0]},
| " outlet_area=a3,\n", | ||
| " pump_gain=k2,\n", | ||
| " gamma_complement=1.0 - gamma2,\n", | ||
| " initial_values={\"prev_h\": [h3_init]},\n", |
There was a problem hiding this comment.
The UpperTank component requires an initial value for pump_voltage to avoid a None type error during the first simulation tick.
initial_values={"prev_h": [h3_init], "pump_voltage": [V2_0]},
| " initial_values={\n", | ||
| " \"internal_recycle\": [initial_internal],\n", | ||
| " \"return_sludge\": [initial_return],\n", | ||
| " },\n", |
There was a problem hiding this comment.
The AnoxicReactor component is missing an initial value for the influent input. Since step() immediately uses self.influent to calculate the mixed liquor state, the simulation will crash on the first tick if this is not provided.
initial_values={
"internal_recycle": [initial_internal],
"return_sludge": [initial_return],
"influent": [_base_influent(params)],
},
| " TrafficLight(name=\"light-a\", junction=\"A\", green_duration=GREEN, yellow_duration=YELLOW, red_duration=RED),\n", | ||
| " TrafficLight(name=\"light-b\", junction=\"B\", green_duration=GREEN, yellow_duration=YELLOW, red_duration=RED, offset=light_b_offset),\n", |
There was a problem hiding this comment.
The TrafficLight components require an initial value for the time input. In the first tick, self.time will be None, and the call to int(self.time) in the step() method will raise a TypeError.
| " TrafficLight(name=\"light-a\", junction=\"A\", green_duration=GREEN, yellow_duration=YELLOW, red_duration=RED),\n", | |
| " TrafficLight(name=\"light-b\", junction=\"B\", green_duration=GREEN, yellow_duration=YELLOW, red_duration=RED, offset=light_b_offset),\n", | |
| TrafficLight(name="light-a", junction="A", green_duration=GREEN, yellow_duration=YELLOW, red_duration=RED, initial_values={"time": [0]}), | |
| TrafficLight(name="light-b", junction="B", green_duration=GREEN, yellow_duration=YELLOW, red_duration=RED, offset=light_b_offset, initial_values={"time": [0]}), |
| " Junction(name=\"junction-a\", junction_name=\"A\", congestion_threshold=15),\n", | ||
| " Junction(name=\"junction-b\", junction_name=\"B\", congestion_threshold=15),\n", |
There was a problem hiding this comment.
The Junction components require initial values for time and signal. While signal might default to a safe check, float(self.time) in step() will crash if time is None.
| " Junction(name=\"junction-a\", junction_name=\"A\", congestion_threshold=15),\n", | |
| " Junction(name=\"junction-b\", junction_name=\"B\", congestion_threshold=15),\n", | |
| Junction(name="junction-a", junction_name="A", congestion_threshold=15, initial_values={"time": [0], "signal": ["red"]}), | |
| Junction(name="junction-b", junction_name="B", congestion_threshold=15, initial_values={"time": [0], "signal": ["red"]}), |
| " Bus(\n", | ||
| " name=bus_ids[i], bus_id=bus_ids[i],\n", | ||
| " departure_tick=i * DEPARTURE_HEADWAY_TICKS,\n", | ||
| " signal_positions=signal_positions,\n", | ||
| " perturbation_stop=PERTURBATION_STOP if (apply_perturbation and bus_ids[i] == PERTURBATION_BUS) else None,\n", | ||
| " perturbation_ticks=PERTURBATION_TICKS if (apply_perturbation and bus_ids[i] == PERTURBATION_BUS) else 0,\n", | ||
| " rng_seed=rng_seed + i * 100,\n", | ||
| " )\n", |
There was a problem hiding this comment.
The Bus component requires an initial value for the tick input. The step() method performs int(self.tick), which will fail with a TypeError on the first tick if initial_values are not provided.
Bus(
name=bus_ids[i], bus_id=bus_ids[i],
departure_tick=i * DEPARTURE_HEADWAY_TICKS,
signal_positions=signal_positions,
perturbation_stop=PERTURBATION_STOP if (apply_perturbation and bus_ids[i] == PERTURBATION_BUS) else None,
perturbation_ticks=PERTURBATION_TICKS if (apply_perturbation and bus_ids[i] == PERTURBATION_BUS) else 0,
rng_seed=rng_seed + i * 100,
initial_values={"tick": [0]},
)
| " # Anti-windup: only integrate when the output is not saturated\n", | ||
| " if u_raw == u_clamped:\n", | ||
| " self._integral[i] += e * self._dt\n", |
There was a problem hiding this comment.
The anti-windup logic here is a bit too restrictive. It only allows integration when the output is exactly equal to the clamped value. A more robust approach is to allow integration if the output is not saturated, OR if the output is saturated but the error has a sign that would move the output away from the limit. This prevents the integral term from getting 'stuck' at the limit when the error changes sign.
# Anti-windup: only integrate if not saturated, or if integration moves output away from limit
if (self._v_min < u_raw < self._v_max) or (u_raw <= self._v_min and e > 0) or (u_raw >= self._v_max and e < 0):
self._integral[i] += e * self._dt
| " if phase_in_cycle > SIGNAL_GREEN_FRAC * 90.0:\n", | ||
| " red_wait = 90.0 - phase_in_cycle\n", | ||
| " distance = max(0.0, distance - BUS_SPEED * red_wait)\n", |
There was a problem hiding this comment.
The traffic signal delay logic implemented here is memoryless. If a bus hits a 'red' light (i.e., distance is reduced to 0), it stays at its current position. However, in the next tick, it re-rolls the probability of passing. This means the wait time follows a geometric distribution based on SIGNAL_GREEN_FRAC rather than a deterministic signal cycle. This contradicts the 'realistic variance' claim in the docstring and can lead to buses being stuck for unrealistic durations or passing immediately despite a 'red' state.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Benchmark comparison for |
1 participant
Summary
Add more examples and use cases for typical process control/industrial simulation problems.
Changes
These examples were primarily researched and implemented by Copilot.