Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#37: Add control mixin and Controller interface #158

Conversation

mstoelzle
Copy link
Contributor

@mstoelzle mstoelzle commented Jul 27, 2022

Motivation

Many usage cases of PyElastica involve prototyping controllers. Often that involves knowing the state of multiple systems or even the state of the entire simulator and use this state information to apply appropriate (external) forces and torques to the various systems to achieve a certain control task.

Considered alternatives

Using the external forcing mixins is insufficient for these use-cases, as it only allows knowledge of one system state at a time and analogue only one system can be controlled at a time. To circumvent these issues, complex and custom workarounds would need to be applied involving multiprocessing or pointers to persistent memory of system objects.

Usage

Devise a custom controller:

from elastica.controllers import ControllerBase
from elastica.typing import SystemType

class CustomController(ControllerBase):
    def __init__(self, systems: Dict[str, SystemType], step_skip: int, *args, **kwargs):
        super().__init__(systems=systems, step_skip=step_skip, *args, **kwargs)

    def apply_forces(self, systems: Dict[str, SystemType], time: float):
        for system_name, system in systems.items():
            system.external_forces += np.ones(shape=system.external_forces.shape)

    def apply_torques(self, systems: Dict[str, SystemType], time: float):
        for system_name, system in systems.items():
            system.external_torques += np.ones(shape=system.external_torques.shape)

Apply controller to simulator:

simulator.control(
        systems={"rod1": rod1, "rod2": rod2, "cylinder1": cylinder1}
).using(CustomController, step_skip=1000)

Example

I added the example of a pendulum tracking a sphere to the ControllerCases in pendulum_tracking_sphere.py.
Here, we initialize a (planar) pendulum as a cylinder in PyElastica, which is only allowed to rotate around the z-axis. The pendulum can be actuated by applying a torque around the z-axis in the inertial frame to the pendulum. We additionally simulate a sphere. This sphere moves in a circular trajectory around the z-axis at a fixed angular velocity, which is unknown to the controller. Now, we construct the PendulumTrackingController as a PID controller tracking the movement of the sphere. We compute the error between the polar angle of the sphere with the polar angle of the pendulum to output a torque acting on the pendulum to track the sphere as closely as possible:

pendulum_tracking_sphere_example_xy.mp4
pendulum_tracking_sphere_example.mp4

This example nicely illustrates the functionality of the new control interface, as this control problem requires access to the state of two simulate systems to compute appropriate forces / torques.

TODOs

  • Add tests
  • Add example
  • Add documentation

Related to:

#37

@mstoelzle
Copy link
Contributor Author

@armantekinalp Here is a first prototype of the controller interface. I mainly took the external forces mixin and adjusted it to the purposes of the control task. Can you please already have a look at the interface?

The next step will be to devise an illustrative example. Do you guys have any nice ideas for simple control tasks involving at least two different systems?

@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2022

Codecov Report

Merging #158 (4801f03) into update-0.3.0 (d0cb959) will decrease coverage by 0.88%.
The diff coverage is 32.60%.

@@               Coverage Diff                @@
##           update-0.3.0     #158      +/-   ##
================================================
- Coverage         87.62%   86.74%   -0.89%     
================================================
  Files                43       45       +2     
  Lines              2820     2866      +46     
  Branches            368      374       +6     
================================================
+ Hits               2471     2486      +15     
- Misses              328      359      +31     
  Partials             21       21              
Impacted Files Coverage Δ
elastica/wrappers/__init__.py 100.00% <ø> (ø)
elastica/wrappers/control.py 26.19% <26.19%> (ø)
elastica/controllers.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0cb959...4801f03. Read the comment docs.

@skim0119 skim0119 marked this pull request as draft July 27, 2022 19:44
@mstoelzle mstoelzle changed the title [WIP] #37: Add control mixin and Controller interface #37: Add control mixin and Controller interface Jul 27, 2022
@mstoelzle
Copy link
Contributor Author

mstoelzle commented Jul 27, 2022

I updated the initial post of this PR with the example of a pendulum tracking a sphere.

@skim0119 @armantekinalp From my side this PR is ready except for the missing tests. Can you please have a look what you guys might be missing in this PR or if you would like to have some implementation changed?

@mstoelzle
Copy link
Contributor Author

I updated the initial post of this PR with the example of a pendulum tracking a sphere.

@skim0119 @armantekinalp From my side this PR is ready except for the missing tests. Can you please have a look what you guys might be missing in this PR or if you would like to have some implementation changed?

@skim0119 @armantekinalp @bhosale2 Do you have an update on reviewing the proposed mixin and interface?

@bhosale2 bhosale2 linked an issue Aug 7, 2022 that may be closed by this pull request
4 tasks
@skim0119 skim0119 marked this pull request as ready for review August 10, 2022 04:08
@skim0119
Copy link
Collaborator

@skim0119 @armantekinalp @bhosale2 Do you have an update on reviewing the proposed mixin and interface?

@mstoelzle Hi -- Sorry for taking some time to answer your question. Our original idea for Controller is somewhat abstract, and we haven't had time to concretely determine the user interface. We are positively considering your approach, but we just want to double-check with our original plans and use cases. We are surveying other collaborators' code to see how this controller concept will integrate, so it will take a couple more days. I'll make sure we answer your question within a week.

@skim0119 skim0119 changed the base branch from update-0.3.0 to update-0.3.1 August 15, 2022 19:52
@skim0119
Copy link
Collaborator

skim0119 commented Aug 15, 2022

@mstoelzle Hi -- Thank you for your patience. We have some criteria we would like to include on the controller. Combined with your example script above and your question on #37, we believe the feature of controller should provide the following.

  • Ability to view/compute forces and torques referencing other systems.
  • Ability to pass action externally.
  • Ability to apply on multiple system (We might add this features to other modules later)

Some concerns we have addressed with your current implementation are:

  • Inside the module, we don't want to expose the system's variable other than the controlling system.

We have come up with a rough concept and interface to address above, and we would like to know if this covers the features you want.

Interface

controller = simulator.add_controller(
    # Active systems we control
    #   The purpose is to group, not to interact rod1 and rod2
    #   Control rule will be applied to every system specified.
    systems: Union[List[SystemType], Dict[Systemtype]] = [rod1, rod2]
).using(
    CustomController,
    passive_systems: Dict[str, SystemType]={"target": sphere1},  # Provide read-only views
    action_shape: Optional[Tuple[int]]=(17,),                    # External parameters
    step_skip: int=1000
)

# External access
parameters = [np.ones(17,), np.zeros(17,)]
controller.set_action(parameters)

Conceptually

from typing import Protocol
from elastica.typing import SystemType


class ControllerBase:
    def get_state(self, tag:str, query: List[str]) -> Dict[str,np.ndarray]:
        state = {}
        for key in query:
            state[key] = self.passive_systems[tag][key]  # This will be read-only
        return state

    def set_action(self, ...):
        ...


class CustomController(ControllerBase):
    def __init__(
        self,
        system: Union[SystemType, List[SystemType], Dict[SystemType]],
        passive_systems: Dict[str, SystemType],
        step_skip: int,
        *args,
        **kwargs
    ):
        super().__init__(
            systems=system,
            step_skip=step_skip,
            passive_systems=passive_systems,
            *args,
            **kwargs
        )

    def compute_force(self, my_velocity, target_velocity, action):
        ...

    def apply_forces(self, system: SystemType, action: np.ndarray, time: float):
        target_velocity = self.get_state("target", ["velocity_collection"])
        system.external_forces += compute_force(
            system.velocity_collection, target_velocity, action)

    def apply_torques(self, system: SystemType, action: np.ndarray, time: float):
        ...

In the end, we believe the current limitation comes from difficulties to handle multiple system instances at once, and this syntax should be able to resolve the issue. Some part must be included in the backend wrapper, such as converting passive_systems to return read-only views, or setting actions. @armantekinalp @bhosale2 Feel free to edit if I missed something.

@mstoelzle
Copy link
Contributor Author

mstoelzle commented Aug 16, 2022

We have come up with a rough concept and interface to address above, and we would like to know if this covers the features you want.

@skim0119 Thanks a lot for this proposal. While I do think it is already an improvement over the current options in the master branch (in particular by having access to more system states through the passive_systems variable), it still has multiple drawbacks and pain-points and would make it hard for me to implement what I need (although it probably would be possible).

Major comment

In many applications involving multi-body system control, we need to know the system state of multiple systems (this is possible in this proposal through the passive_systems variable), then compute the optimal control input for multiple active systems at the same time (Please note that the number of passive systems does not necessarily need to match the number of active systems), and then finally apply the computed control inputs to the active system. While the proposed concept would work well for the special case, where the control input of each active system can be computed independently, this is not possible / efficient in many cases.

Let's take the example of serial manipulator or parallel robots: here we usually jointly optimize the control inputs for all actuators controlling all links / joints. If I want to implement this following the proposed interface, I would need to add a separate controller for each joint. But as I have to optimize the entire serial manipulator, I would need to always optimize for all systems, and then only apply the control input for one joint at a time. This has two major disadvantages:

  1. A lot of computations are wasted, as I need to evaluate the optimal control problem $n_a$ times (where $n_a$ is the number of active systems), instead of just evaluating it once.
  2. I need to either copy the entire controller code or add if conditions to choose which joint I am controlling.

I am not sure what is your reasoning behind adding this strict requirement of applying the same control input to all active systems this controller is assigned to. At least in my perspective, this is unnecessarily limiting and restrictive.

I see some potential benefits of distinguishing between active_systems and passive_systems (I guess it should prevent some bugs?), but the user needs to have the freedom to apply a separate control input to each active system.

With respect to this (independent of the actions - see below), I would propose an implementation as follows:

from typing import Protocol
from elastica.typing import SystemType


class CustomController(ControllerBase):
    def __init__(
        self,
        active_system: Dict[str, SystemType],
        passive_systems: Dict[str, SystemType] = {},
        step_skip: int,
        *args,
        **kwargs
    ):
        super().__init__(
            active_systems=active_systems,
            step_skip=step_skip,
            passive_systems=passive_systems,
            *args,
            **kwargs
        )

    def compute_optimal_control_inputs(self, my_velocities, target_velocity, action):
        pass

    def apply_forces(self, active_systems:  Dict[str, SystemType], action: np.ndarray, time: float):
        target_velocity = self.get_state("target", ["velocity_collection"])
        my_velocities = [system.velocity_collection for system in active_systems.values()]
        control_inputs = compute_optimal_control_inputs(
            my_velocities, target_velocity, action)
        i = 0
        for system_name, active_system in active_systems.items():
             active_system.external_forces += control_inputs[i]
             i += 1

    def apply_torques(self, active_systems:  Dict[str, SystemType], action: np.ndarray, time: float):
        pass

Minor comment

I guess this would depend on the specific implementation, but would apply_forces and apply_torques need to be evaluated once for each (active) system? This would be computationally inefficient, if the output is the same anyway.

Questions

I also have some questions concerning the proposed set_actions implementation. I don't think I am fully grasping the motivation and concept yet:

  1. Is the idea that the user can change control parameters (e.g. actions) while the simulation is running? Or should the user only pass actions before the simulation is started?
  2. A list of action ndarrays (with two items) is passed to the set_action method. Is now the controller called for each action in this list separately at each time-step?
  3. If the control parameters should be changed externally before the simulation is started, why can the user not directly access the controller object attributes? E.g. controller.parameters = np.ones((17, ))

I would be grateful if you could explain the concept behind these actions in more detail.

@skim0119
Copy link
Collaborator

skim0119 commented Aug 16, 2022

@mstoelzle Thank you for your response. I'll try to make some comments and answer some questions.

On Major Comments,

I'm not quite sure if I understood your example case correctly, but I still think what you have described can be implemented with the previous schematic, maybe with some minor additional features.

Let's take the example of serial manipulator or parallel robots: here we usually jointly optimize the control inputs for all actuators controlling all links / joints. If I want to implement this following the proposed interface, I would need to add a separate controller for each joint. But as I have to optimize the entire serial manipulator, I would need to always optimize for all systems, and then only apply the control input for one joint at a time. This has two major disadvantages:

Isn't this the whole purpose of having passive_system? You should be able to pass whichever state information you need to compute your control law at once. If your concern is the redundancy of computation within apply_force and apply_torques, we can provide pre-processing and post-processing steps that are called before and after. Pre-step to do joint computation, post-step to do logging or callback, for example.

Roughly,

class ControllerBase: # ABC or Protocol
    def pre_step(self, ...): # Called before apply_force/apply_torques
        ...

    def post_step(self, ...):  # Called after apply_force/apply_torques
        ...

    def get_state(self, tag:str, query: List[str]) -> Dict[str,np.ndarray]:
        state = {}
        for key in query:
            state[key] = self.passive_systems[tag][key]  # This will be read-only
        return state

    def set_action(self, ...):
        ...

    ...

class CustomController(ControllerBase):
    def pre_step(self):
        state: Dict[str, np.ndarray] = self.get_state(...)
        self.forces, self.torques = nb_control_law(state)

    def apply_forces(self, system: SystemType, action: np.ndarray, time: float):
        system.external_forces += self.forces[system]

I need to either copy the entire controller code or add if conditions to choose which joint I am controlling.

I'm not sure what you mean. If you don't have conditioning, isn't it same as applying same law for each joint?

I am not sure what is your reasoning behind adding this strict requirement of applying the same control input to all active systems this controller is assigned to. At least in my perspective, this is unnecessarily limiting and restrictive.

This is not a restriction, nor strict. It is up to users to write their own apply_forces and apply_torques, and the above code is just an example. Beside, if the user is going to have different control law for different rods, it should be added as a different controller.

If user needs active system's state, they can also pass those to passive_system parameters.

the user needs to have the freedom to apply a separate control input to each active system.

What is difference between having two separate controllers in this case? If concern is some joint computation, can you maybe implement using pre_step above?


Maybe there is something I'm missing, but I believe we are getting closer to what is needed. Could you maybe provide some example scripts that you are having trouble with?

Regarding Minor Comment

  • Mainly, we don't want apply_forces and apply_torques to have a unique syntax here compare to other modules (forcing, etc), especially because we don't expect users to care exactly how/when those functions are called. We want to keep consistency in the syntax.
  • In that regard, we understand that this does not always produce clean code. For example, if we have N-rod influenced under gravity, we need N forcing objects. It would be convenient if one gravitational forcing object can handle multiple rods.
  • We are discussing this issue, and we are considering expanding the syntax to support a list of rods, but we want to handle it separately along with how we handle block memory.

Regarding Action

This is probably our original intention of making a controller, and it is a somewhat different purpose than what you have proposed. We thought if this controller will serve as an interface for more generalized robotics/optimization problems, we need external online controller for user to intervene within the simulation. This is why the concept goes along with the environment.
We have other collaborators requesting the feature. The code I wrote above is just a rough sketch. If needed, I'll probably be the one implementing the feature directly on top of your controller implementation. I don't expect this to be part of this PR, but we surely want this feature to be part of the controller.

The idea is that our current implementation only supports passive or prescribed force/torque input. There are ways to change activation during the simulation, but it is not so clean and clear to use. Within this context, we thought the controller should act as a variable lambdas within the simulator, exposed to user, used by forcing/environment, such that user can pass the activation.

Just to answer your questions,

Is the idea that the user can change control parameters (e.g. actions) while the simulation is running? Or should the user only pass actions before the simulation is started?

Some parameters are controllable by the user while the simulation is running.

A list of action ndarrays (with two items) is passed to the set_action method. Is now the controller called for each action in this list separately at each time-step?

(conceptually) set_action method can be used to set the action/activation values, and those values will be used to compute actuation until next action is set.

If the control parameters should be changed externally before the simulation is started, why can the user not directly access the controller object attributes? E.g. controller.parameters = np.ones((17, ))

That way is also fine; I just thought having a setter method can include pre-check or callbacks. At this point, I don't see much difference between controller.parameters = np.ones((17,)) and controller.set_action(np.ones((17,))).

@mstoelzle
Copy link
Contributor Author

mstoelzle commented Aug 16, 2022

Regarding actions

Ok thanks, I understand the purpose a bit better then.

parameters = [np.ones(17,), np.zeros(17,)]
controller.set_action(parameters)

I don't understand why you are passing here a list of numpy arrays, and then apply_forces only takes one numpy array as an action.
Shouldn't the code look like this:

controller.set_action(np.ones(17,))

and then maybe at another time-step:

controller.set_action(np.zeros(17,))

@skim0119 It also needs to made clear in apply_forces that the action is optional and might be None, if set_action is not called.

As a side comment connected to enable the use of external online controllers: for these online controllers to work, it might be important feature to specify at which real-time rate the simulator should run. Some robotics frameworks such as ROS run at a fixed clock, and any interface between PyElastica and ROS-based nodes will be very difficult to implement without having the confidence at which real-time rate (e.g. how many simulation time-steps per second of real-time) the simulator is running.

@armantekinalp
Copy link
Contributor

As a side comment connected to enable the use of external online controllers: for these online controllers to work, it might be important feature to specify at which real-time rate the simulator should run. Some robotics frameworks such as ROS run at a fixed clock, and any interface between PyElastica and ROS-based nodes will be very difficult to implement without having the confidence at which real-time rate (e.g. how many simulation time-steps per second of real-time) the simulator is running.

@mstoelzle will it be enough to pass time-step of simulation to the ControllerClass

@skim0119
Copy link
Collaborator

Regarding actions

Ok thanks, I understand the purpose a bit better then.

parameters = [np.ones(17,), np.zeros(17,)]
controller.set_action(parameters)

I don't understand why you are passing here a list of numpy arrays, and then apply_forces only takes one numpy array as an action. Shouldn't the code look like this:

controller.set_action(np.ones(17,))

and then maybe at another time-step:

controller.set_action(np.zeros(17,))

I passed the list of arrays, thinking first np.ones(17,) is passed to first rod and second np.zeros(17,) is passed to second rod. As you said, .set_action will be called at different time-steps.

@skim0119 It also needs to made clear in apply_forces that the action is optional and might be None, if set_action is not called.

The parameter passed to apply_forces/apply_torques will be passed from backend, and previously set action will be passed. It is optional, allowing None, in case when user don't need external controllability and pass None for action_shape. This API can probably be improved, I'm open to ideas.

As a side comment connected to enable the use of external online controllers: for these online controllers to work, it might be important feature to specify at which real-time rate the simulator should run. Some robotics frameworks such as ROS run at a fixed clock, and any interface between PyElastica and ROS-based nodes will be very difficult to implement without having the confidence at which real-time rate (e.g. how many simulation time-steps per second of real-time) the simulator is running.

Yes, I agree, and it is definitely one of the reasons why this controller-environment implementation had been slow. The original project didn't anticipate online/external communication, rather it started as physics/mechanics simulation. The clock cycle is not guaranteed to be consistent at this point.

@armantekinalp
Copy link
Contributor

I don't understand why you are passing here a list of numpy arrays, and then apply_forces only takes one numpy array as an action.

Here the idea is there might be more than one active system in the simulator and we pass a list containing actions for each rod. @skim0119 can confirm

@mstoelzle
Copy link
Contributor Author

mstoelzle commented Aug 16, 2022

Here the idea is there might be more than one active system in the simulator and we pass a list containing actions for each rod. @skim0119 can confirm

Ok, I get what you mean and I am supportive of this purpose. However, this is really inconsistent:

  1. For (online) actions, you allow a separate action (e.g. forces) to be defined for each active system
  2. For other controllers implemented in the apply_forces, you do not allow separate outputs (e.g. forces) for each active system. Here, according to the proposal, each active system needs to receive the same force from the controller

This kind of doesn't make sense. I would therefore suggest to remove the requirement that each active system needs to have the same apply_forces method implemented ;)

@mstoelzle
Copy link
Contributor Author

mstoelzle commented Aug 16, 2022

Regarding the major comment

Maybe there is something I'm missing, but I believe we are getting closer to what is needed. Could you maybe provide some example scripts that you are having trouble with?

@skim0119 Ok let us consider a planar serial manipulator (e.g. a robotic arm) consisting of three (rigid) links and three joints. Let's also assume that we already have trained an RL controller with observation space 18 (e.g. full position and velocity of each link) and actuation space of 3 (e.g. torque to be applied to each joint). This requires knowledge of all three system (e.g. links) states (which is given by your proposal), but also the actuation of three systems, as the RL controller returns 3 torques. Now, we need to implement the following steps:

  1. We collect the state of each system (e.g. link). Then, we concatenate these states all into one array and pass it to the RL controller.
  2. We evaluate the RL controller and it will return an array of 3 (e.g. the torque for each actuated joint).
  3. For each (actuated) joint, we need to apply opposite external torques to the links.

If I want to implement this according to your proposal for the controller interface, I would need to do it likes this:

  1. Attach separate controllers to each link
  2. In each of these three controllers, I need to collect the states of all systems by making use of the passive_system functionality and evaluate the RL controller.
  3. But now, I only take the output of the RL controller, which belongs the link the controller is attached to. All other computed torques, I throw away.

In this example, we had to evaluate the RL controller 3x instead of just 1x time, because of the requirements of the controller interface. If we could define multiple active_systems, we could evaluate the RL controller just once and directly apply the relevant torques to the active systems.

This is not a restriction, nor strict. It is up to users to write their own apply_forces and apply_torques, and the above code is just an example. Beside, if the user is going to have different control law for different rods, it should be added as a different controller.

I don't think this is true. As apply_forces is called from the wrapper, the user can never have multiple active systems in one controller and apply separate torques to each active system. The interface of the apply_forces and apply_torques is fixed by the wrapper / module.

@skim0119
Copy link
Collaborator

skim0119 commented Aug 16, 2022

@mstoelzle -- Something along this line?

class ModelProtocol(Protocol):
    def predict(self, X: np.ndarray) -> np.ndarray:
        ...

class SerialManipulatorController(ControllerBase):
    def __init__(self, model: ModelProtocol, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.model = model
        self.z = np.array([0. ,0. ,1.])

    def _convert_state_array(self, state) -> np.ndarray:
        ...

    def pre_step(self, active_system, passive_system, action, time):
        state = self.get_state(0, ['position_collection', 'velocity_collection'])
        state = self._convert_state_array(state)
        actuations = self.model.predict(state) # Assuming in-order of links
        actuations.append(0)

        self.base_torques, self.end_torques = {}, {}
        for idx, system in enumerate(active_system):
            self.base_torques[system] = actuations[idx] * self.z
            self.end_torques[system] = -actuations[idx+1] * self.z

    def apply_torques(self, system: SystemType, action: np.ndarray, time: float):
        system.external_torques[...,0] += self.base_torques[system]
        system.external_torques[...,-1] += self.end_torques[system]

link1: SystemType = CosseratRod(...)
link2: SystemType = CosseratRod(...)
link3: SystemType = CosseratRod(...)

path:str = "<model path>"
RL_Model = tf.keras.model.load_model(path)

simulator.add_controller(
    [link1, link2, link3]
).using(
    SerialManipulatorController,
    model: ModelProtocol=RL_Model
    passive_systems=[link1, link2, link3],
    step_skip=1000
)

@mstoelzle
Copy link
Contributor Author

@skim0119 Thanks for going through the effort and coding the implementation of this example :)

I think this would work and cover the technical needs of my project. From a code-style and UI perspective, I still think it is a bit overly complicated and could be implemented in a lighter way.

I still don't quite understand what is your motivation for implementing it this way? What is the benefit of adding this pre_step compared to passing the list / dict of active systems to the main step (e.g. apply_forces)?

Mainly, we don't want apply_forces and apply_torques to have a unique syntax here compare to other modules (forcing, etc), especially because we don't expect users to care exactly how/when those functions are called. We want to keep consistency in the syntax.

If the reason is that the users are likely expecting a specific kind of syntax for apply_forces and apply_torques without this iterable of active systems, why not name the method differently, such as:

  • run_controller()
  • control()
  • run()
  • apply_control_input()
  • etc.
    ?
    This method signature would then look like this:
def run_controller(self, active_systems: Dict[str, SystemType], actions: Optional[Dict[str, np.ndarray]], time: float):
        pass

@skim0119
Copy link
Collaborator

skim0119 commented Aug 17, 2022

@mstoelzle

I still think it is a bit overly complicated and could be implemented in a lighter way.

Our focus is more geared towards delivering a "generalizable" solver, and it is prioritized over an "clean-looking" solver. We want this feature to be benefiting other collaborator's projects and any future projects as well, which means we want the design of the pipeline/functionals modularized and application-agnostic.

If the reason is that the users are likely expecting a specific kind of syntax for apply_forces and apply_torques without this iterable of active systems, why not name the method differently,

Your run_controller does exactly the same thing as pre_step. I'm not super attached to the name pre_step because it sounds like a step in a Callback, but we can refactor that after implementation.

We don't want to change the structure of apply_forces and apply_torques, because keeping the interface the same helps us modify/maintain the backend wrappers later on. More variation downstream will increase the workload for future maintenance.

@skim0119 skim0119 changed the base branch from update-0.3.1 to feature/controller October 2, 2022 02:34
@bhosale2
Copy link
Collaborator

Closing it for now due to its inactive status. @armantekinalp we can re-open it in the future if work on this resumes again.

@bhosale2 bhosale2 closed this Feb 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interface for Environment and Control
5 participants