Skip to content

Commit e6447b4

Browse files
committed
Fix controller duplication in TimeNodes.merge()
Remove duplicate _controllers append that caused each controller to be called twice per simulation time node. Closes #959
1 parent 9cf3dd4 commit e6447b4

2 files changed

Lines changed: 53 additions & 10 deletions

File tree

rocketpy/simulation/flight.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3743,7 +3743,6 @@ def merge(self):
37433743
tmp_dict[time]._controllers += node._controllers
37443744
tmp_dict[time].callbacks += node.callbacks
37453745
tmp_dict[time]._component_sensors += node._component_sensors
3746-
tmp_dict[time]._controllers += node._controllers
37473746
except KeyError:
37483747
# If the node does not exist, add it to the dictionary
37493748
tmp_dict[time] = node

tests/integration/simulation/test_flight.py

Lines changed: 53 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
)
1515
@patch("matplotlib.pyplot.show")
1616
# pylint: disable=unused-argument
17-
def test_all_info(mock_show, request, flight_fixture):
17+
def test_all_info(request, flight_fixture):
1818
"""Test that the flight class is working as intended. This basically calls
1919
the all_info() method and checks if it returns None. It is not testing if
2020
the values are correct, but whether the method is working without errors.
@@ -38,7 +38,7 @@ def test_all_info(mock_show, request, flight_fixture):
3838
# RK23 is unstable and requires a very low tolerance to work
3939
# pylint: disable=unused-argument
4040
def test_all_info_different_solvers(
41-
mock_show, calisto_robust, example_spaceport_env, solver_method
41+
calisto_robust, example_spaceport_env, solver_method
4242
):
4343
"""Test that the flight class is working as intended with different solver
4444
methods. This basically calls the all_info() method and checks if it returns
@@ -69,7 +69,7 @@ def test_all_info_different_solvers(
6969

7070

7171
@patch("matplotlib.pyplot.show")
72-
def test_hybrid_motor_flight(mock_show, flight_calisto_hybrid_modded): # pylint: disable=unused-argument
72+
def test_hybrid_motor_flight(flight_calisto_hybrid_modded): # pylint: disable=unused-argument
7373
"""Test the flight of a rocket with a hybrid motor. This test only validates
7474
that a flight simulation can be performed with a hybrid motor; it does not
7575
validate the results.
@@ -85,7 +85,7 @@ def test_hybrid_motor_flight(mock_show, flight_calisto_hybrid_modded): # pylint
8585

8686

8787
@patch("matplotlib.pyplot.show")
88-
def test_liquid_motor_flight(mock_show, flight_calisto_liquid_modded): # pylint: disable=unused-argument
88+
def test_liquid_motor_flight(flight_calisto_liquid_modded): # pylint: disable=unused-argument
8989
"""Test the flight of a rocket with a liquid motor. This test only validates
9090
that a flight simulation can be performed with a liquid motor; it does not
9191
validate the results.
@@ -102,7 +102,7 @@ def test_liquid_motor_flight(mock_show, flight_calisto_liquid_modded): # pylint
102102

103103
@pytest.mark.slow
104104
@patch("matplotlib.pyplot.show")
105-
def test_time_overshoot(mock_show, calisto_robust, example_spaceport_env): # pylint: disable=unused-argument
105+
def test_time_overshoot(calisto_robust, example_spaceport_env): # pylint: disable=unused-argument
106106
"""Test the time_overshoot parameter of the Flight class. This basically
107107
calls the all_info() method for a simulation without time_overshoot and
108108
checks if it returns None. It is not testing if the values are correct,
@@ -131,7 +131,7 @@ def test_time_overshoot(mock_show, calisto_robust, example_spaceport_env): # py
131131

132132

133133
@patch("matplotlib.pyplot.show")
134-
def test_simpler_parachute_triggers(mock_show, example_plain_env, calisto_robust): # pylint: disable=unused-argument
134+
def test_simpler_parachute_triggers(example_plain_env, calisto_robust): # pylint: disable=unused-argument
135135
"""Tests different types of parachute triggers. This is important to ensure
136136
the code is working as intended, since the parachute triggers can have very
137137
different format definitions. It will add 3 parachutes using different
@@ -273,7 +273,7 @@ def test_eccentricity_on_flight( # pylint: disable=unused-argument
273273

274274

275275
@patch("matplotlib.pyplot.show")
276-
def test_air_brakes_flight(mock_show, flight_calisto_air_brakes): # pylint: disable=unused-argument
276+
def test_air_brakes_flight(flight_calisto_air_brakes): # pylint: disable=unused-argument
277277
"""Test the flight of a rocket with air brakes. This test only validates
278278
that a flight simulation can be performed with air brakes; it does not
279279
validate the results.
@@ -293,8 +293,52 @@ def test_air_brakes_flight(mock_show, flight_calisto_air_brakes): # pylint: dis
293293
assert air_brakes.prints.all() is None
294294

295295

296+
def test_controllers_not_duplicated_in_merge():
297+
"""Regression test for issue #959. Verifies TimeNodes.merge() does
298+
not duplicate controller references during node merging.
299+
"""
300+
from rocketpy.simulation.flight import Flight
301+
TimeNodes = Flight.TimeNodes
302+
303+
# Create two time nodes at the same time, each with one controller
304+
def dummy(t, y, sol, s):
305+
pass
306+
307+
nodes = TimeNodes()
308+
nodes.add_node(0, [], [dummy], [])
309+
nodes.add_node(0, [], [dummy], [])
310+
nodes.sort()
311+
nodes.merge()
312+
313+
# After merge, two controllers should remain (one from each node)
314+
merged = nodes.list[0]._controllers
315+
assert len(merged) == 2, f"Expected 2 controllers, got {len(merged)}"
316+
317+
# Repeat with three nodes to verify accumulation scales
318+
nodes2 = TimeNodes()
319+
nodes2.add_node(0, [], [dummy], [])
320+
nodes2.add_node(0, [], [dummy], [])
321+
nodes2.add_node(0, [], [dummy], [])
322+
nodes2.sort()
323+
nodes2.merge()
324+
assert len(nodes2.list[0]._controllers) == 3
325+
326+
# Controllers at different times should remain separate
327+
nodes3 = TimeNodes()
328+
nodes3.add_node(0, [], [dummy], [])
329+
nodes3.add_node(1, [], [dummy], [])
330+
nodes3.add_node(0, [], [dummy], [])
331+
nodes3.sort()
332+
nodes3.merge()
333+
assert len(nodes3.list) == 2
334+
# t=0 should have 2 controllers (one from each node), t=1 should have 1
335+
times = {round(n.t, 7): len(n._controllers) for n in nodes3.list}
336+
assert times[0.0] == 2
337+
assert times[1.0] == 1
338+
339+
296340
@patch("matplotlib.pyplot.show")
297-
def test_initial_solution(mock_show, example_plain_env, calisto_robust): # pylint: disable=unused-argument
341+
def test_initial_solution(example_plain_env, calisto_robust): # pylint: disable=unused-argument
298342
"""Tests the initial_solution option of the Flight class. This test simply
299343
simulates the flight using the initial_solution option and checks if the
300344
all_info method returns None.
@@ -339,7 +383,7 @@ def test_initial_solution(mock_show, example_plain_env, calisto_robust): # pyli
339383

340384

341385
@patch("matplotlib.pyplot.show")
342-
def test_empty_motor_flight(mock_show, example_plain_env, calisto_motorless): # pylint: disable=unused-argument
386+
def test_empty_motor_flight(example_plain_env, calisto_motorless): # pylint: disable=unused-argument
343387
flight = Flight(
344388
rocket=calisto_motorless,
345389
environment=example_plain_env,

0 commit comments

Comments
 (0)