BUG: Air Brakes Deployment Constant at Post Process

MateusStano commented 2 months ago

Pull request type

[x] Code changes (bugfix, features)

Checklist

[x] Tests for the changes have been added (if needed)
[x] Docs have been reviewed and added / updated
[x] Lint (black rocketpy/ tests/) has passed locally
[x] All tests (pytest tests -m slow --runslow) have passed locally
[x] 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:

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.
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
[x] 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 https://github.com/RocketPy-Team/RocketPy/pull/295/commits/291d479124bba13599040cd7a673d41cf3f8f721. Maybe @Gui-FernandesBR knows what this was for?

codecov[bot] commented 2 months ago

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 :warning:

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

MateusStano commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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 commented 1 month ago

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

MateusStano commented 1 month ago

@Gui-FernandesBR its ready

Gui-FernandesBR commented 1 month ago

@Gui-FernandesBR its ready

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

RocketPy-Team / RocketPy