From 3798c19a3555ea8df6c175892d25761232664728 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 3 Aug 2026 04:49:45 +0800 Subject: [PATCH] BUG: make every step_simulation call advance the flight A call that landed on a phase boundary advanced the phase index, left the new phase uninitialised and returned, so it moved neither t nor y_sol. A caller counting one step per call, which is what the Balloon Popping Challenge environment does, then ran ahead of the flight's own clock. The phase transition now carries on into the new phase in the same call, so the only path that returns without advancing is the one that has just set finished, and by then there is nothing left to advance. Measured on flight_calisto: 5 calls with 2 that did not advance, against 3 calls with none. Final t is 48.4363 either way, so the same flight is being walked in fewer calls rather than a different one. Ignoring whitespace the change is a while loop and a continue; the rest of the diff is the indentation that loop adds. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- rocketpy/simulation/flight.py | 188 +++++++++--------- tests/unit/simulation/test_step_simulation.py | 38 ++++ 2 files changed, 135 insertions(+), 91 deletions(-) diff --git a/rocketpy/simulation/flight.py b/rocketpy/simulation/flight.py index 43fb6210c..49da800f8 100644 --- a/rocketpy/simulation/flight.py +++ b/rocketpy/simulation/flight.py @@ -879,97 +879,78 @@ def step_simulation(self): if state["finished"]: return - phase_index = state["phase_index"] - if phase_index >= len(self.flight_phases) - 1: - state["finished"] = True + # One call has to leave the flight further along than it found it, so a + # call that lands on a phase boundary carries on into the new phase + # instead of returning. Only the finish path below returns without + # advancing, and by then there is nothing left to advance. + while True: + phase_index = state["phase_index"] + if phase_index >= len(self.flight_phases) - 1: + state["finished"] = True - self.post_process_simulation() - self.initialize_prints_plots() - return - - phase = self.flight_phases[phase_index] - - # Determine maximum time for this flight phase - phase.time_bound = self.flight_phases[phase_index + 1].t - - # Initialize phase only once - if not state["phase_initialized"]: - # Evaluate callbacks - for callback in phase.callbacks: - callback(self) + self.post_process_simulation() + self.initialize_prints_plots() + return - # Create solver for this flight phase - self.function_evaluations.append(0) + phase = self.flight_phases[phase_index] - phase.solver = self._solver( - phase.derivative, - t0=phase.t, - y0=self.y_sol, - t_bound=phase.time_bound, - rtol=self.rtol, - atol=self.atol, - max_step=self.max_time_step, - min_step=self.min_time_step, - ) - - # Initialize phase time nodes - self.__setup_phase_time_nodes(phase) + # Determine maximum time for this flight phase + phase.time_bound = self.flight_phases[phase_index + 1].t - state["phase_initialized"] = True - state["node_index"] = 0 + # Initialize phase only once + if not state["phase_initialized"]: + # Evaluate callbacks + for callback in phase.callbacks: + callback(self) - # Check if current phase is fully processed - if state["node_index"] >= len(phase.time_nodes) - 1: - state["phase_index"] += 1 - state["phase_initialized"] = False - state["node_index"] = 0 - return # Move to next phase on next call + # Create solver for this flight phase + self.function_evaluations.append(0) - node_index = state["node_index"] - node = phase.time_nodes[node_index] + phase.solver = self._solver( + phase.derivative, + t0=phase.t, + y0=self.y_sol, + t_bound=phase.time_bound, + rtol=self.rtol, + atol=self.atol, + max_step=self.max_time_step, + min_step=self.min_time_step, + ) - # Determine time bound for this time node - node.time_bound = phase.time_nodes[node_index + 1].t - phase.solver.t_bound = node.time_bound + # Initialize phase time nodes + self.__setup_phase_time_nodes(phase) - if self.__is_lsoda: - phase.solver._lsoda_solver._integrator.rwork[0] = phase.solver.t_bound - phase.solver._lsoda_solver._integrator.call_args[4] = ( - phase.solver._lsoda_solver._integrator.rwork - ) + state["phase_initialized"] = True + state["node_index"] = 0 - phase.solver.status = "running" + # Check if current phase is fully processed + if state["node_index"] >= len(phase.time_nodes) - 1: + state["phase_index"] += 1 + state["phase_initialized"] = False + state["node_index"] = 0 + continue # the new phase is initialised below, in this same call - # Feed required parachute and discrete controller triggers - # TODO: parachutes should be moved to controllers - for callback in node.callbacks: - callback(self) + node_index = state["node_index"] + node = phase.time_nodes[node_index] - for controller in node._controllers: - controller( - self.t, - self.y_sol, - self.solution, - self.sensors, - self.env, - ) + # Determine time bound for this time node + node.time_bound = phase.time_nodes[node_index + 1].t + phase.solver.t_bound = node.time_bound - # Placeholder for parachute triggers in step simulation, which is currently not migrated + if self.__is_lsoda: + phase.solver._lsoda_solver._integrator.rwork[0] = phase.solver.t_bound + phase.solver._lsoda_solver._integrator.call_args[4] = ( + phase.solver._lsoda_solver._integrator.rwork + ) - while phase.solver.status == "running": - # Execute solver step, log solution and function evaluations - phase.solver.step() - self.solution += [[phase.solver.t, *phase.solver.y]] - self.function_evaluations.append(phase.solver.nfev) + phase.solver.status = "running" - # Update time and state - self.t = phase.solver.t - self.y_sol = phase.solver.y - if self.verbose: - print(f"Current Simulation Time: {self.t:3.4f} s", end="\r") - logger.debug("Current Simulation Time: %3.4f s", self.t) + # Feed required parachute and discrete controller triggers + # TODO: parachutes should be moved to controllers + for callback in node.callbacks: + callback(self) - for controller in self._continuous_controllers: + for controller in node._controllers: controller( self.t, self.y_sol, @@ -978,25 +959,50 @@ def step_simulation(self): self.env, ) - if self.__check_simulation_events(phase, phase_index, node_index): - break # Stop if simulation termination event occurred + # Placeholder for parachute triggers in step simulation, which is currently not migrated - # Process overshootable time nodes if enabled - if self.time_overshoot and self.__process_overshootable_nodes( - phase, phase_index, node_index - ): - break + while phase.solver.status == "running": + # Execute solver step, log solution and function evaluations + phase.solver.step() + self.solution += [[phase.solver.t, *phase.solver.y]] + self.function_evaluations.append(phase.solver.nfev) - # If controlled flight, post process must be done on sim time - # Post-process controllers if needed - if self._controllers: - phase.derivative(self.t, self.y_sol, post_processing=True) + # Update time and state + self.t = phase.solver.t + self.y_sol = phase.solver.y + if self.verbose: + print(f"Current Simulation Time: {self.t:3.4f} s", end="\r") + logger.debug("Current Simulation Time: %3.4f s", self.t) - if node._component_sensors: - u_dot = phase.derivative(self.t, self.y_sol) - self.__measure_sensors(node._component_sensors, u_dot) + for controller in self._continuous_controllers: + controller( + self.t, + self.y_sol, + self.solution, + self.sensors, + self.env, + ) - state["node_index"] += 1 + if self.__check_simulation_events(phase, phase_index, node_index): + break # Stop if simulation termination event occurred + + # Process overshootable time nodes if enabled + if self.time_overshoot and self.__process_overshootable_nodes( + phase, phase_index, node_index + ): + break + + # If controlled flight, post process must be done on sim time + # Post-process controllers if needed + if self._controllers: + phase.derivative(self.t, self.y_sol, post_processing=True) + + if node._component_sensors: + u_dot = phase.derivative(self.t, self.y_sol) + self.__measure_sensors(node._component_sensors, u_dot) + + state["node_index"] += 1 + return def __setup_phase_time_nodes(self, phase): """Set up time nodes for the current phase. diff --git a/tests/unit/simulation/test_step_simulation.py b/tests/unit/simulation/test_step_simulation.py index c7ac43df9..11936918a 100644 --- a/tests/unit/simulation/test_step_simulation.py +++ b/tests/unit/simulation/test_step_simulation.py @@ -177,6 +177,44 @@ def _step_with_roll(env, rocket, command, max_steps=100000): return flight, steps +class TestEveryCallAdvances: + """A call has to leave the flight further along than it found it. + + The phase transition used to return without touching ``t`` or ``y_sol``, + leaving the new phase to be initialised on the call after. A caller that + counts a step per call, which is what the Balloon Popping Challenge + environment does, then has its own clock ahead of the flight's. + """ + + def test_no_call_returns_without_advancing(self, flight_calisto): + stepped = _stepped_twin(flight_calisto) + stalled = [] + calls = 0 + while not stepped._step_state["finished"]: + before = stepped.t + stepped.step_simulation() + calls += 1 + if not stepped._step_state["finished"] and stepped.t <= before: + stalled.append(calls) + + assert not stalled, f"calls {stalled} of {calls} did not advance" + + def test_a_transition_is_absorbed_rather_than_costing_a_call(self, flight_calisto): + """The control for the test above, which returning early on every call + would also pass. More than one phase has to actually be visited.""" + stepped = _stepped_twin(flight_calisto) + _, phases_seen = _run_stepped(stepped) + + assert len(phases_seen) > 1 + + def test_the_flight_still_ends_where_simulate_ends(self, flight_calisto): + """Absorbing the transition must not skip the node it was standing on.""" + stepped = _stepped_twin(flight_calisto) + _run_stepped(stepped) + + np.testing.assert_allclose(stepped.t, flight_calisto.t, rtol=1e-8, atol=1e-10) + + class TestControlledStepSimulation: """Injecting an actuator command between steps must move the trajectory.