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

Integrate bigym training pipeline #1

Merged
merged 12 commits into from
Aug 5, 2024

Conversation

Yusufma03
Copy link
Contributor

This PR integrates the BiGym training pipeline with robobase and verifies the pipeline with ACT

@@ -218,6 +218,7 @@ def __init__(

@staticmethod
def transform_from_tanh(action, action_stats, min_max_margin):
action = action.clip(-1.0, 1.0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this is because the action head of ACT is just a simple linear layer, which doesn't transform policy output to [-1, 1], and it may go out of bounds during execution so we have to clip it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to clip the action in def act?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richielo Would this break Genima?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it could be better to keep this line, and don't modify ACT as we're not sure if it will break genima or not

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think it would be better to leave this line here. As we always assume actions to be bounded by [-1, 1] given transformations in robobase.

@younggyoseo
Copy link
Contributor

It could be good to check again if we should include low_dim_obs normalization for running ACT in BiGym

@Yusufma03
Copy link
Contributor Author

It could be good to check again if we should include low_dim_obs normalization for running ACT in BiGym

Found that the official implementation actually does the normalization, but we didn't do that in our previous code. I can add a wrapper here to align with the official one. But it shouldn't matter too much I guess.

Copy link
Contributor

@younggyoseo younggyoseo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me with some minor comments

@robobase0907 robobase0907 merged commit 5e5705a into robobase-org:master Aug 5, 2024
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.

4 participants