Convert ts.integrate param units#587
Open
curtischong wants to merge 2 commits into
Open
Conversation
add run cmd simplify repro new repro repro fix use their model integrator fixes revert to sevennet cleanup get structures in memory better integrator definitions cleanup better test names more cleanup cleanup more cleanup cleanup cleanup cleanup delete repro file fix other params better name use constants cleanup
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
We currently are not converting the parameters in ts.integrate (they are supposed to accept normal units)
Note: this is in contrast to the internal api (direct integrator functions like
ts.npt_nose_hoover_isotropic_stepwhere they expect the units to already be converted).This PR goes through all the params and adds a type annotation to these args in the integrator functions to properly convert the units.
Note: additional kwargs like
a = kwargs.get("momenta")in https://github.com/TorchSim/torch-sim/blob/main/torch_sim/integrators/nvt.py#L344 and https://github.com/TorchSim/torch-sim/blob/main/torch_sim/integrators/npt.py#L1733 do not have annotated conversion param like the rest. I think this is fine since users are not manually initing the momenta for individual atoms in the ts.integrate api. - they're most likely loaded in from another simulation (so I think they'll have the proper units)This PR is to fix the issue outlined in #579 where a user was changing the settings for a simulation and since we didn't properly convert the units to internal MetalUnits, their simulation was unstable. This change fixes the issue noticed in their md workflow
Checklist
Before a pull request can be merged, the following items must be checked: