BUG: Air Brakes Deployment Constant at Post Process

[MateusStano]

Pull request type

• Code changes (bugfix, features)

Checklist

• Tests for the changes have been added (if needed)
• Docs have been reviewed and added / updated
• Lint (black rocketpy/ tests/) has passed locally
• All tests (pytest tests -m slow --runslow) have passed locally
• CHANGELOG.md has been updated (if relevant)

Current behavior

The calculations of post-processed variable (i.e. accelerations, forces and moments) are considering only the last state of the air brakes, instead of reconsidering its deployment level.

New behavior

Fixing this issue was quite challenging since we wanted to maintain the freedom of the Controller class without big/breaking changes. The complexity here has to do with the fact that whatever object is being controlled (in this case the air brakes) has a state that varies throughout the simulation. There are two ways to go about dealing with this:

1. Save the control inputs at every control function call
   • Requires that we save exactly what we need from each possible controllable object.
   • For the air brakes, we need only the deployment level.
   • This would force us to have clearly defined what is controllable and what is not, and we would need a way for the user to specify what he is controlling and what he is not. This would be a different approach from the current one, where anything is controllable inside the controller function.
2. Save accelerations and forces during simulation (run post-processing during simulation)
   • This increases simulation time by one udot call plus the time to append values to all necessary arrays per time step
   • There is no need to do post process after simulation because everything calculated is a @cached_property

The second option was implemented. The following was changed on the code:

• A _reset method was added to the AirBrakes class that resets the deployment level to its initial value. This guarantees that running the air brakes will behave equally if two Flight initializations are run consecutively
• A warning was added for when a simulation with controllers is run without time_overshoot=False
• A __retrieve_arrays method was created that unites retrieve_acceleration_arrays and retrieve_temporary_values_arrays into only one loop and optimizes it. This was done so that the in-simulation post-processing for controllers could be done in only one udot call.

Breaking change

• Yes
• No

Additional information

One thing was changed that changes simulation results. The accelerations used to always be initialized with.

    ax, ay, az = [[0, 0]], [[0, 0]], [[0, 0]]
    alpha1, alpha2, alpha3 = [[0, 0]], [[0, 0]], [[0, 0]]

I do not know what this was for and it seems wrong at a first glance. I changed it to be initialized with empty lists instead and all tests are passing.

The initialization with [[0, 0]] was introduced in #295, commit 291d479. Maybe @Gui-FernandesBR knows what this was for?

[codecov]

Codecov Report

Attention: Patch coverage is 94.73684% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 73.33%. Comparing base (fc6804c) to head (d730b40).
Report is 3 commits behind head on develop.

Files	Patch %	Lines
rocketpy/simulation/flight.py	93.75%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #586      +/-   ##
===========================================
+ Coverage    73.31%   73.33%   +0.02%     
===========================================
  Files           57       57              
  Lines         9436     9448      +12     
===========================================
+ Hits          6918     6929      +11     
- Misses        2518     2519       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Gui-FernandesBR]

Good PR, thanks for the implementations.

I've added some comments, and I'm open to discuss all of them before approving your PR.

Regarding the initialization of acceleration arrays with zeros, I believe it was done to avoid errors when initializing a flight using another flight as the initial solution.
Could you check if all the examples in the examples page are running correctly after you PR?

Also, sorry for being the CHANGELOG guru again, but could you please add your PR to the list? It should become an habit to us.

[Gui-FernandesBR]

Trying to build the docs here... look what I've got:

File ~\Documents\github\RocketPy\rocketpy\mathutils\function.py:3388, in funcify_method.<locals>.funcify_method_decorator.__get__(self, instance, owner)
   3384 except KeyError:
   3385     # If cache is not ready, create it
   3386     try:
   3387         # Handle methods which return Function instances
-> 3388         val = self.func(instance).reset(*args, **kwargs)
   3389     except AttributeError:
   3390         # Handle methods which return a valid source
   3391         source = self.func(instance)

File ~\Documents\github\RocketPy\rocketpy\simulation\flight.py:2449, in Flight.stream_velocity_x(self)
   2445 @funcify_method("Time (s)", "Freestream Velocity X (m/s)", "spline", "constant")
   2446 def stream_velocity_x(self):
   2447     """Freestream velocity X component as a Function of time."""
   2448     stream_velocity_x = np.column_stack(
-> 2449         (self.time, self.wind_velocity_x[:, 1] - self.vx[:, 1])
   2450     )
   2451     return stream_velocity_x

ValueError: operands could not be broadcast together with shapes (2066,) (2067,)

[MateusStano]

Trying to build the docs here... look what I've got:

This seems to be due to terminate_on_apogee making the simulation stop before calling the extra udot with post processing. I will fix it soon

[Gui-FernandesBR]

This PR may present a conflict with the #581 , given that both deal with the postprocessing methods.
I think we might need to merge some ideas here.

[MateusStano]

@Gui-FernandesBR ready for a new review!

This PR may present a conflict with the #581 , given that both deal with the postprocessing methods.
I think we might need to merge some ideas here.

The best option is to merge your PR first, and then I can change what is necessary here. Its not a big conflict

[Gui-FernandesBR]

@MateusStano please let me know when this gets ready for a re-review

[MateusStano]

@Gui-FernandesBR its ready

[Gui-FernandesBR]

@Gui-FernandesBR its ready

Ok, I want to test it locally before approving, so I will take a little longer time to review it.