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

Fixes reset of sensor drift inside the RayCaster sensor #1821

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

zoctipus
Copy link
Contributor

@zoctipus zoctipus commented Feb 9, 2025

Description

The drift in raycaster didn't update when reset. This MR now updates it correctly:

before:

self.drift[env_ids].uniform_(*self.cfg.drift_range)

now:

self.drift[env_ids] = self.drift[env_ids].uniform_(*self.cfg.drift_range)

Fixes #1820

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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

@@ -110,7 +110,7 @@ def reset(self, env_ids: Sequence[int] | None = None):
if env_ids is None:
env_ids = slice(None)
# resample the drift
self.drift[env_ids].uniform_(*self.cfg.drift_range)
self.drift[env_ids] = self.drift[env_ids].uniform_(*self.cfg.drift_range)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this required for uniform_? I think that might be an inplace operation already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for being careful, I have checked again in my debugger

>self.drift[env_ids]
tensor([[0., 0., 0.],
        [0., 0., 0.],
        [0., 0., 0.],
        ...,
        [0., 0., 0.],
        [0., 0., 0.],
        [0., 0., 0.]], device='cuda:0')
>self.drift[env_ids].uniform_(*self.cfg.drift_range)
tensor([[-0.0657,  0.0741, -0.0464],
        [-0.0895,  0.0378,  0.0849],
        [ 0.0414, -0.0142,  0.0127],
        ...,
        [ 0.0647,  0.0327, -0.0896],
        [ 0.0268, -0.0399, -0.0197],
        [ 0.0034, -0.0036,  0.0639]], device='cuda:0')
>self.drift[env_ids]
tensor([[0., 0., 0.],
        [0., 0., 0.],
        [0., 0., 0.],
        ...,
        [0., 0., 0.],
        [0., 0., 0.],
        [0., 0., 0.]], device='cuda:0')
        

as you see the self.drift is not updated after the uniform_ operation, though it returns a uniformly sampled values

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 do remember styles in pytorch where they use _ to indicate in place operation, it is indeed strange how it didn't work here. My torch version is 2.5.1+cu118

Copy link
Contributor

@Mayankm96 Mayankm96 Feb 13, 2025

Choose a reason for hiding this comment

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

The problem isn't with the in-place operation. Rther if you are indexing a torch tensor with explicit indices, it makes a "copy" of that tensor. So the original tensor isn't getting assigned but rather the copy of it.

Nice catch!

@zoctipus
Copy link
Contributor Author

zoctipus commented Feb 12, 2025

@kellyguo11
Upon further research, I found out this indexing is causing the issue

Comparing below two cases

Case 1:

>self.drift
tensor([[0., 0., 0.],
        ...,
        [0., 0., 0.]], device='cuda:0')
>self.drift.uniform_(*self.cfg.drift_range)
tensor([[-0.0657,  0.0741, -0.0464],
        ...,
        [ 0.0034, -0.0036,  0.0639]], device='cuda:0')
>self.drift
tensor([[-0.0657,  0.0741, -0.0464],
        ...,
        [ 0.0034, -0.0036,  0.0639]], device='cuda:0')

Case 2:

>self.drift[env_ids]
tensor([[0., 0., 0.],
        ...,
        [0., 0., 0.]], device='cuda:0')
>self.drift[env_ids].uniform_(*self.cfg.drift_range)
tensor([[-0.0657,  0.0741, -0.0464],
        ...,
        [ 0.0034, -0.0036,  0.0639]], device='cuda:0')
>self.drift[env_ids].     <------------------   change did not take effect
tensor([[0., 0., 0.],
        ...,
        [0., 0., 0.]], device='cuda:0')

it seems like the advance indexing returns coped data instead of of original data's view, and uniform_ indeed changed the data in place, but the change is on copied data!

@Mayankm96 Mayankm96 changed the title bug fix: field drift in raycaster didn't update when reset, now update correctly Fixes reset of sensor drift inside the RayCaster sensor Feb 12, 2025
@Mayankm96 Mayankm96 added the bug Something isn't working label Feb 12, 2025
@Mayankm96 Mayankm96 merged commit 92f8930 into isaac-sim:main Feb 13, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug Report] Field self.drift in Raycaster does not update correct when reset is called
3 participants