Adds basic validation tests for scale-based randomization ranges

[louislelay]

Description

While working on a task I made a tiny typo in the scale randomization range for the stiffness-gain parameter. The robot still spawned, but its behaviour was utterly bizarre. It only took a minute to spot the mistake, yet it made me realize we have no guard-rails for this sort of edge case.

This PR introduces a lightweight check that verifies, when operation=="scale", the lower bound is non-negative and the upper bound is not smaller than the lower one. Right now I cover the most common parameters (stiffness, damping, mass, tendon gains, etc.), basically anything that must stay positive to make physical sense.

If you’d like the same safeguard applied to other parameter types just let me know and I’ll happily extend the patch.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

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

[ooctipus]

Hi @louislelay,

This is amazing! thank you so much on moving toward this direction, please let me know if you need any my help : )). More than happy to support it!

[louislelay]

Hey @ooctipus, thanks for the kind message haha!

I was thinking we should add this kind of safeguard to the other parameters in events as well. For some parameters, negative values should be allowed, but whenever there’s both a lower and an upper limit, we need to verify that one is greater than the other, not just for scale, but for every operation. Should we merge this PR as it is and open a new one for the rest, or do you think we should handle everything here?

[ooctipus]

@louislelay
I think we can merge this PR as it is, it does deal with all scale cases, so I think it is pretty standalone.
But if you think the guard in other places are almost identical to this one, then maybe we put them into one PR. I am happy either way : ))

[louislelay]

Well, I think we should merge this one in this case, @ooctipus! I’ll open another PR later this week once I have a bit more time to work on it :)
I’ll mention you when I open the other PR so you can review it!

[Mayankm96]

Is there a perf cost for these sanity checks? I wonder if they should only happen "once" on construction to not lose performance because of the introduced branching checks.

[ooctipus]

My understanding is that these operations are for mode: startup use case only, if it is mode: reset, then it is definitely a problem...

[louislelay]

Hey @Mayankm96 and @ooctipus, actually, there are a few of these that are used sometimes as startup and sometimes as reset, so that’s definitely an issue. These current changes then don’t really make sense anymore, so I should probably look as Mayank suggested for another way to check at construction.

If you have any pointers about that, I’d gladly take them!

[ooctipus]

Hey @louislelay Thank you for checking that there are examples of using them as mode:reset, I was under impression that only mode:startup was the usage in isaaclab. But I was wrong.

For making a sanity check at construction time rather than call time
please take a look at here. example of sanity check in constructor

You can move all the check into the __init__ and check cfg.params passed in from init input arguments

[louislelay]

Thanks @ooctipus, that’s exactly the kind of pointer I was looking for :)

I’ll take the time to do it tomorrow -- I don’t have any GPU at hand right now!

[louislelay]

I've updated a few things but haven’t tested every function during use yet. I’ll do that later tonight, so it’s not ready for review for now!

[louislelay]

Hey @Mayankm96 and @ooctipus, sorry for the delay, it’s been a really busy week. I tried the OpenAI Shadow Hand environment since it uses all the changed events. I realized parameters shouldn’t be checked if they’re not used, so I updated the tests accordingly and added one to ensure the user can only choose scale, add, or abs.

[ooctipus]

Everything looks good!!
tests all passing, last thing
@louislelay Could you please update CHANGELOG and extension.toml in isaaclab?

[louislelay]

Hey @ooctipus, thank you! I just updated those :)