Repository navigation
Conversation
|
Hi @CompRhys happy to work on this in 2 weeks! Defending my thesis next Friday so slammed with that but will have time to work on this and a few other things if interested after the fact. |
|
Best of luck! the @ was just to try ensure you still got credit for the work that you did here on neb |
dabf177 to
58b3b8e
Compare
|
To get this to work a concept of |
|
Hey, I'm not super familiar with NEB. For instance, |
orionarcher
left a comment
There was a problem hiding this comment.
I agree that we do need a group_idx but I don't think this is the right approach to operationalizing it.
As I understand it, if we split the state before autobatching then we limit the batch size to the group size, which will throttle performance in the case of small groups.
Instead, I think we need to modify the logic of the autobatchers so that they can either split by system_idx or split by group_idx. Then we can re-use the same logic to pack the maximum number of groups into memory that we use to pack the maximum number of systems into memory. This will require a deeper rewrite to the autobatchers though.
I suggest having a autobatcher property on instantiation that selects "group_idx" or "system_idx" as the batching index. Or could be a bool batch_by_group=False.
I spent a fair amount of time thinking about this last year and I think everything should transition over nicely, no blockers on an architectural level.
I was trying to get it such that group_idx was the thing that autobatchers worked off and that the default behavior would be single system groups. I think that's cleaner than having switching logic? |
There was a problem hiding this comment.
All reported issues were addressed across 12 files
Shadow auto-approve: would not auto-approve because issues were found.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Merged latest
|
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would require human review. New NEB workflow and optimizer-group support thread group_idx through SimState, FIRE, autobatchers, and runners, making this a broad public/core API change with design tradeoffs.
Tip: Get a Slack DM when cubic finishes reviewing your PRs. Connect Slack
View guided diff | Turn on auto-fix | Re-trigger cubic



This revives @mstapelberg's NEB work from #90 and adds optimizer-group (
group_idx) support for running batched Nudged Elastic Band calculations cleanly alongsideSimState, optimizers, runners, and autobatchers.Summary of Changes
torch_sim/workflows/neb.py):NEB,neb_init,neb_step,validate_endpoints,interpolate_path,compute_tangents,calculate_neb_forces, andassemble_pathwith climbing-image NEB, FIRE (vv_fireandase_fire) / GD optimizer support,FixAtomsconstraint propagation, per-imageTrajectoryReporterfilenames, and_NEBModelAdapterso all optimizers receive constraint-masked NEB forces.group_idx) acrossSimState, FIRE, Runners, and Autobatchers:SimState: Addedgroup_idxand_group_attributessupport with invariant validation in__post_init__,__setattr__,pop,from_state, slicing/remapping, and docstrings explaining whygroup_idxexists (n_systemsvsn_groups).fire_init/fire_step: Adapted reductions (batched_vdot(..., n_systems=n_groups)) and per-group FIRE state (dt,alpha,n_pos) to operate per optimizer group while preserving zero-GPU-sync execution.BinningAutoBatcher&InFlightAutoBatcher+optimize/integraterunners: Supported group-level batching, lazyIterablestreaming without input mutation,estimate_max_memory_scalerper-group aggregation, andrestore_original_orderfor multi-system groups.examples/tutorials/nudged_elastic_band.py(linked indocs/tutorials/index.rst) comparing batched TorchSim NEB paths against sequential ASE NEB.tests/workflows/test_neb.py,tests/test_state.py, andtests/test_autobatching.py.