-
Notifications
You must be signed in to change notification settings - Fork 70
Fix #293: State to device side effects #297
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
Fix #293: State to device side effects #297
Conversation
|
Hi @orionarcher and @thomasloux, saw your message on the slack channel and wanted to make my contribution, let me know if any edits are required! |
|
by the way, not sure if we need this kind of tests in the repo |
orionarcher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sacredvoid! Welcome to the project. One small request but otherwise looks good!
torch_sim/state.py
Outdated
| attrs = vars(state) | ||
| # Use copy.copy to create a shallow copy of the attributes dict | ||
| # This avoids modifying the input state's internal dictionary | ||
| attrs = copy.copy(vars(state)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not copy here. The behavior of .to() below is such that it will create a copy if the device is different.
|
I'd agree with @thomasloux that we can get rid of most of the tests. I'd just test |
|
@orionarcher hey, i've updated the PR with requested changes, let me know if it's good to go, thank you |
orionarcher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job @sacredvoid, LGTM! Will merge when tests pass.
|
Hey, I would remove the tests and modify the description. Now, the IS to change the input state. I would also directly return state to prevent create a new dataclass object with all its variables pointing to another dataclass object |
Summary
Fix: Resolve SimState side effects in device/dtype conversion
Problem: SimState.to(), concatenate_states(), and initialize_state() were modifying input states instead of creating new instances.
Solution:
Fixed _state_to_device() to use copy.copy(vars(state)) instead of direct reference
Updated functions to create new instances without side effects
Key Change:
Impact: Fixes Issue #293, eliminates unexpected state mutations, maintains backward compatibility.
Testing: Added test_fix_demo.py to verify all operations preserve input states unchanged.
Checklist
Before a pull request can be merged, the following items must be checked: