-
Notifications
You must be signed in to change notification settings - Fork 74
Feat/allow different temperatures ts integrate #367
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
base: main
Are you sure you want to change the base?
Feat/allow different temperatures ts integrate #367
Conversation
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.
Awesome! This is a nice feature. I'd suggest adding a bit more testing fo the different temperature shapes though.
torch_sim/runners.py
Outdated
| # This assumes that in case n_systems == n_steps, the user wants to apply | ||
| # different temperatures per system, not per step. | ||
| if temps.shape[0] == initial_state.n_systems: | ||
| # Interpret as single-step multi-system temperatures → broadcast over steps | ||
| return temps.unsqueeze(0).expand(n_steps, -1) # (n_steps, n_systems) |
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.
I honestly might just throw a well-documented error here and demand the user provide a 2D tensor if n_systems == n_steps. It's a rare edge case and the consequences of incorrectly guessing the default is potentially many hours of wasted debugging.
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.
I added a warning, let me know if you want to be more strict and raise an error
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.
Sounds good to me
|
Note for me: I need to add a test to check that it works with the autobatcher |
|
@orionarcher I modified the autobatcher.bin_index which was a list[dict[int, float]] which I think is just the results of the original use of the binpacking from the first version of autobatcher #39. I changed the bin_index to actually be list[list[indices]]. Otherwise |
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.
Would be nice to have a test with a 2D array but otherwise LGTM
|
Ready to merge @thomasloux? |

Summary
Change ts.integrate to accept temperature of size (n_system,), so that its system can have its own temperature.
Checklist
Before a pull request can be merged, the following items must be checked: