Moves delay actuator functionality to IdealPDActuator

[jtigue-bdai]

Description

Ths PR:

• Moves delay functionality from DelayedPDActuator to IdealPDActuator. This provides optional functionality for all inherited actuators like DCMotor

Type of change

• New feature (non-breaking change which adds functionality)

Screenshots

Please attach before and after screenshots of the change if applicable.

Checklist

• I have run the pre-commit checks with ./isaaclab.sh --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• I have updated the changelog and the corresponding version in the extension's config/extension.toml file
• I have added my name to the CONTRIBUTORS.md or my name already exists there

[Mayankm96]

Could we please split this up to into different MRs? The update hook and motor strength need a better look/discussion to understand.

[jtigue-bdai]

Could we please split this up to into different MRs? The update hook and motor strength need a better look/discussion to understand.

@Mayankm96 I have removed the motor_strength and update_params

[ooctipus]

LGTM with the delay-mechanism. Just a curious question, what types of real world motor is delay-mechanism most applicable?

[jtigue-bdai]

Any real world system is going to have a time delay of 1 time step in their control rate due to the time to measure, transmit to the controller, calculate and transmit to the actuator. Usually this is at high rates like 500-1000 hz so the policy rates are usually significantly slower that this delay and are often ignored during training.

[ooctipus]

Seems like test regarding source/isaaclab/test/actuators/test_ideal_pd_actuator.py is failing, could you please take a look at it @jtigue-bdai

[ZiwenZhuang]

Is it possible to implement these features into Mixin classes, so that potential applications can be implemented via multiple inheritance?

Otherwise, "ideal" is not "ideal", "implicit" will be not "implicit".

It will be extremely difficult to manage and control the data flow if multiple features are merged in a single class.

[ooctipus]

Hi @ZiwenZhuang Thank you for the feedback, Can you provide a concrete example why it makes difficult?

If you want multiple inheritance, you can always default delay to false to preserve the non-delay behavior of ideal pd. Also I am not quite sure this PR is affecting implicit actuator as it doesn't touch implicit actuator at all.

We'd happy to give a second thought and reconsider our approach if your example does introduces diffculties, through I am not understanding it super clearly.

[ZiwenZhuang]

@ooctipus Hi, thank you for your attention.

As I helped fixing one of the bugs in ObservationManager when dealing with observation modifiers, history buffer and other features, too many features are included into one single code implementation.

It is indeed a way of resolving diamond inheritance problem, but it is a start of mixing all features together.

All collaborators involving this code have to make sure that the code still works when setting delay to false, e.g. if there is no DelayBuffer presented in the actuator. This introduce potential collaboration problems.

By "Ideal" I would expect only PD torque computation without anything else. When I was new to IsaacLab, I implemented another delayed action / actuator mechanism myself and found out there is already a DelayedPDActuator in IsaacLab. lol

I believe it is important to let the user knows what they have used as intuitive as possible (by just looking at the class name).

[ooctipus]

@ZiwenZhuang I see, so your concern is that the class name IdealPDActuator doesn't convey the idea that there could be a delay mechanism associate with it. I can resonate your concern, though, I don't believe this PR necessary break this concept as the default min_delay and max_delay are all 0. Means that if you don't know about delay, the Ideal PD should behave as if delay mechanism doesn't exist

I think you also brought up a second good point about maintainability of the code when many feature starts to clots together, and think what you have described in observation manager is definitely one of the case when many features are convoluted. I think it is right that we should be aware of this issue and start refactor old code and reconsider the design as we accommodating more features.

Say if we instead of write the raw code of delay mechanism in IdealPD, but make delay mechanism a helper utility class, and compose delay mechism into IdealPD through API call, maybe do you think it will be more clear?

Let me use seudo code to illustrate,

@configclass
class DelayCfg:
   
    class_type: type[Delay] = Delay
   
    min_delay: int = 0

    max_delay: int = 0


@configclass
class IdealPDActuatorCfg(ActuatorBaseCfg):

    class_type: type = actuator_pd.IdealPDActuator
   
    delay: delay_cfg = util.DelayCfg



class IdealPDActuator:

    def __init__(self):

      if cfg.delay_cfg:
            self.delay_util = cfg.delay_cfg.class_type(cfg.delay_cfg)
            
       ..... other code ....

    def reset(self):
        if self.cfg.delay_cfg:
              self.delay_util.reset()
         
         ..... other code ....

    def compute(self):
         control_action = self.delay_utils.compute(control_action)
         
         ..... other code ....

Maybe a helper class like this can be more easier to incorporate features instead of writing raw code?

[ZiwenZhuang]

@ooctipus
Hmmm, this pseudo code seems making Actuator a manager...., which may handles many features such as delay, clipping, dynamic protection, motor saturation, etc...

But the potential risk still exists. In your pseudo code case. All later code must check hasattr(self, "delay_util") if they want to use self.delay_util instance. But I don't believe potential contributors will check these conditions in detail.

Maybe design the Actuator like a manager will make the features less convoluted (just like the modifiers in observation term)?

But that leave the risk of making the code difficult to understand.

[ooctipus]

@ZiwenZhuang

Yes you are totally right, Structure, Conciseness, Features, Clarity, it is very difficult to balance all of them. The move to refactor delay into IdealPD is a move to improve the conciseness so that all explicit actuators inherited from IdealPD can benefit from having or not having delay without we have to mannually create one version under IdealPD and the other version under DelayedPD.

In old approach you could ends up having DCMotorNoDelay and DCMotorDelay, which we will soon explodes the number of combinatoric classes combinations.

As you have suggested, our move toward conciseness may have potentially also made our code Clarity reduced. We could improve structure to meditate it, but as you mentioned again, structure can itself sometime hard to understand. Sometime a genius engineer could find a clever way to compose all component such that it improves all aspects, but that will take a lot of thoughts and inspiration, and many time requires a lot of features to be added first to consider a well-thought solution.

For now, since delay doesn't take much raw code to implemented and behavior can be silent by default, I think this PR should be fine. (Sorry if it has introduced inconvinence in your code). But you are right that we should be aware of the exploding features like ObservationManagers and start considering redesigning the class instead of keep adding more features.

I hope you also somewhat agree with me on this dilemma, and we very appreciate to be told that if there is a design we to improve all aspects of our code.

[ZiwenZhuang]

@ooctipus
In that case, I would recommend using another name rather than IdealPDActuator and put these features in IdealPDActuator's one single subclass. (I just realized motor_strength is another term added to IdealPDActuator)

For example a motor manufacturer wants to write their own motor model, but keep the PD equation exactly consistent with the IdealPDActuator. The default IdealPDActuatorCfg must behave exactly as if there are no additional features.

On the other hand, it would be impossible to remove those features once it is added, for the backward compatibility to the down-stream developers.

From my point of view, a mixin-class design or a manager-style (let's say effort_modifiers in PDActuator class) will be more consistent in the future.

[ooctipus]

Thanks for the suggestion.
Since current design will make motor strength and delay silent by default, I don't see the need for subclassing further to introduce these attributes. motor manufacturer in your example could basically ignore the delay parts of the IdealPDActuator and would achieve exactly what they want as if delay or motor_strength doesn't exist. Beside, subclassing introduces a lot of rigidity and code jump that I don't think subclass is a future proof way to go.

Comparing to subclassing, Mixin style or manager-style sounds more alike a reasonable composible and scalable solution to me. : )). We will make sure to slowly utilize these styles in multiple components in our code base to reduces the convolution ... Thank you for the suggeestions