Skip to content

Conversation

ClemensSchwarke
Copy link
Collaborator

@ClemensSchwarke ClemensSchwarke commented Oct 10, 2025

Validated on the following tasks:

  • Isaac-Velocity-Flat-Anymal-D-v0
  • Isaac-Velocity-Flat-Anymal-D-Recurrent-v0
  • Isaac-Velocity-Flat-Anymal-D-Distillation-v0
  • Isaac-Velocity-Flat-Anymal-D-Distillation-Recurrent-v0

@ClemensSchwarke ClemensSchwarke marked this pull request as ready for review October 13, 2025 09:14
"""

def broadcast_parameters(self):
def broadcast_parameters(self) -> None:

Choose a reason for hiding this comment

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

i personally dont like the -> None type inting anywhere, it feels redundant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Mayankm96 Whats your opinion on this? :D

def reset(
self,
dones: torch.Tensor | None = None,
hidden_states: tuple[torch.Tensor | tuple[torch.Tensor, ...] | None, ...] = (None, None),

Choose a reason for hiding this comment

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

this is such an ugly type 😅
It would feel pretty confusing to me if i were using it for the first time. Either we define a HiddenState type in the memory.py or we need to add some docstrings. I would personally prefer the first one

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mh, I get it, but I am personally not a big fan of defining types. But maybe it still makes sense for this one.

old_mu_batch = old_mu[batch_idx]
old_sigma_batch = old_sigma[batch_idx]

hid_a_batch = None

Choose a reason for hiding this comment

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

does creating this None variables inside the loop add some non negligeble overhead? Probably not right

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would doubt that. Should be fine :)

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.

2 participants