ENH: Alternative for State History #507

MateusStano · 2023-12-08T03:00:31Z

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 --runslow) have passed locally
CHANGELOG.md has been updated (if relevant)

Current behavior

The AirBrakes implementation had some questionable handling of the state attributes. They made the Controller Class work only with air brakes, and their implementation was not scalable to the other classes

New behavior

All air brakes' state related attributes and functions were deleted.

Now, logging of changes is handled inside the Controller class:

Controller class now has an attribute observed_variables, which is a list passed as an argument for the controller_function
observed_variables is initialized with the initial_observed_variables in the class' constructor
The observed_variables gets appended to with whatever the controller_function returns.

These changes make it so no information of the simulation is stored in the Air Brakes class. If the user wanted to see the change of deployed level through time, he would have to return time and the deployed level inside the controller_function and then make the plot himself.

This makes the Controller class much more flexible in terms of usability.

Additional information

I have not updated the docs page yet, but there is an updated example in the notebook

This current implementation makes me question the need for the state_history argument of the controller_function, since the user could just keep what it needs in the observed_variables

Need opinions on this @giovaniceotto @phmbressan

rocketpy/control/controller.py

Gui-FernandesBR

Good work, Stano. Overall you reduced the total number of code line, that's always a good improvement.

I wonder why the tests are not passing... Could you take a look please?

Gui-FernandesBR · 2023-12-09T02:43:38Z

rocketpy/control/controller.py

-            and modified by the controller function.
+        interactable_objects : list
+            A collection of objects that the controller function can access and
+            potentially modify. This list is flexible and can include any Python


"This list is flexible and can include any Python object"

this sentence let me a bit lost here. What would be examples of "any Python object"? What do you mean by that?

Well, the user can pass any type of object in this list. If they want to use an object of a class that they defined they can.

Since these are passed directly to the controller_function, and the controller_function is completely defined by the user, he can pass whatever he wants in here.

I will try to improve the docs a bit here

Gui-FernandesBR · 2023-12-09T02:48:39Z

rocketpy/control/controller.py

-            5. A list containing the objects to be acted upon by the controller.
-               The objects in this list are the same as the objects in the
-               observed_objects list, but they can be modified by the controller.
+            4. `state_history` (list): A record of the rocket's state at each


so the state_history works like the same as the Flight.solution attribute, right?
I wonder if each state could/should be an instance of a class, so we would be saving a list of objects of such class, instead of saving a list of lists.

It is an interesting idea. Maybe being a named tuple would be more fitting

It is something to consider whenever we work on some refactoring in the Flight class

rocketpy/control/controller.py

Gui-FernandesBR · 2023-12-09T02:53:21Z

rocketpy/rocket/rocket.py

+            controller_function=controller_function,
+            sampling_rate=sampling_rate,
+            initial_observed_variables=initial_observed_variables,
+            name=controller_name,
        )
        self.air_brakes.append(air_brakes)


don't we have a self.add_air_brakes(air_brakes) method? These two rows (1263 and 1264) does the same thing but in two different ways.

I don't think I understood what you were saying here

you have used these two code lines:

self.air_brakes.append(air_brakes)

and

self.add_controllers(controller)

My question is: why do you append airbakes but you add controllers? Shouldn't they follow the same architecture? The code could be more intuitive

add_controllers is a separate method that can be used by itself to add any type of controller to the rocket

the add_air_brakes is specific for adding air_brakes. It also adds the controller because it just makes sense for air brakes to be added together with their controller

add_controllers is a separate method that can be used by itself to add any type of controller to the rocket

the add_air_brakes is specific for adding air_brakes. It also adds the controller because it just makes sense for air brakes to be added together with their controller

Now I'm the one who thinks you didn't understand...

The point is that in your code you didn't use add_air_brakes at all, instead you are mutating a list using the self.airbrakes.append().

Imagine when we are on the Rocket class, we don't do this: calisto.aero_surfaces.append(...).
Instead, we do calisto.add_surface(...).
This is important because there are some function calls inside the add_surface method that make the first approach not valid.

add_air_brakes creates the air brakes and their related controller and saves them in their appropriate lists.

Air brakes do not generate any lift, so there is no reason for it to be included in aerodynamic_surfaces or make any center of pressure-related calculations. So it does not call add_surface

Also, air brakes have no position, so it only needs to be saved in a normal list. No need to create a Component object

phmbressan

This PR offers much improved flexibility for controlling different rocket components, well done. I will approve it after some comments I have raised are considered.

Every implementation has advantages and disadvantages, the main comments I made concern these:

Advantages:

Greater flexibility and scalability, does not require history parameters in the controlled objects;
Support for much deeper/complex control.

Disadvantages:

There are no automatically generated plots for control results;
(Perhaps) A bit harder to use/understand when creating a controller from scratch.

rocketpy/control/controller.py

rocketpy/rocket/rocket.py

rocketpy/control/controller.py

Co-authored-by: Pedro Henrique Marinho Bressan <87212571+phmbressan@users.noreply.github.com>

MateusStano · 2023-12-15T22:14:06Z

Advantages:

Greater flexibility and scalability, does not require history parameters in the controlled objects;

Support for much deeper/complex control.

Disadvantages:

There are no automatically generated plots for control results;

(Perhaps) A bit harder to use/understand when creating a controller from scratch.

To sum up, I think we can always add the air brakes plots somehow later. It would have to be something similar to the old implementation (which is not ideal) but without making it necessary for any object that can be controlled to have it as well.

Also, with a good documentation (specially in a usage .rst docs page) the user should not have a problem handling the data

phmbressan

Thanks for your replies @MateusStano this solves the comments I had about this PR. Good job!

giovaniceotto · 2023-12-16T21:29:34Z

@MateusStano, can we merge this PR and then I'll do a final review of #426 directly?

MateusStano added 2 commits December 8, 2023 03:41

ENH: remove state list and add saved returns for controller_function

8ba8202

MNT: controllable_objects to interactable_objects

34622e8

MateusStano added the Controllers Controlling rocket flight methods label Dec 8, 2023

MateusStano requested a review from phmbressan December 8, 2023 03:00

MateusStano self-assigned this Dec 8, 2023

MateusStano requested a review from giovaniceotto as a code owner December 8, 2023 03:00

github-actions bot requested a review from Gui-FernandesBR December 8, 2023 03:00

phmbressan reviewed Dec 8, 2023

View reviewed changes

rocketpy/control/controller.py Outdated Show resolved Hide resolved

ENH: fix initial_observed_values

bf0fb81

Gui-FernandesBR reviewed Dec 9, 2023

View reviewed changes

phmbressan reviewed Dec 15, 2023

View reviewed changes

rocketpy/control/controller.py Show resolved Hide resolved

rocketpy/control/controller.py Outdated Show resolved Hide resolved

rocketpy/rocket/rocket.py Show resolved Hide resolved

rocketpy/control/controller.py Show resolved Hide resolved

Update rocketpy/control/controller.py

ff2e143

Co-authored-by: Pedro Henrique Marinho Bressan <87212571+phmbressan@users.noreply.github.com>

phmbressan approved these changes Dec 16, 2023

View reviewed changes

MateusStano added 2 commits December 18, 2023 03:24

DOC: update docs

260e8d8

TST: update tests

a631b6f

MateusStano merged commit 995b97a into enh/air-brakes Dec 18, 2023
5 of 9 checks passed

MateusStano deleted the enh/new-controller-structure branch December 18, 2023 02:59

Gui-FernandesBR added this to the Release v1.X.0 milestone Feb 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Alternative for State History #507

ENH: Alternative for State History #507

MateusStano commented Dec 8, 2023 •

edited

Loading

Gui-FernandesBR left a comment

Gui-FernandesBR Dec 9, 2023

MateusStano Dec 18, 2023

Gui-FernandesBR Dec 9, 2023

MateusStano Dec 18, 2023

Gui-FernandesBR Dec 9, 2023

MateusStano Dec 18, 2023

Gui-FernandesBR Dec 18, 2023

MateusStano Dec 18, 2023

Gui-FernandesBR Dec 18, 2023

MateusStano Dec 18, 2023

MateusStano Dec 18, 2023

phmbressan left a comment

MateusStano commented Dec 15, 2023

phmbressan left a comment •

edited

Loading

giovaniceotto commented Dec 16, 2023

ENH: Alternative for State History #507

ENH: Alternative for State History #507

Conversation

MateusStano commented Dec 8, 2023 • edited Loading

Pull request type

Checklist

Current behavior

New behavior

Additional information

Gui-FernandesBR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phmbressan left a comment

Choose a reason for hiding this comment

MateusStano commented Dec 15, 2023

phmbressan left a comment • edited Loading

Choose a reason for hiding this comment

giovaniceotto commented Dec 16, 2023

MateusStano commented Dec 8, 2023 •

edited

Loading

phmbressan left a comment •

edited

Loading