Convert ts.integrate param units#587
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
|
The alternative here is simply to delete the concept of the unit convertors within TS that this PR attempts to fix and then leave it up to the user to ensure that they're numerically consistent between their model and integration parameters. @abhijeetgangan would you be opposed to that as a lower maintenance and LLOC solution? |
|
I think we need to keep the metalunits bc it does simplify the formulas. here's an example branch where I asked an ai to remove the units main...curtischong:torch-sim:remove-metal-units. Maybe there's a better way though |
|
I think I'm just hesitant because I'm not familiar with all the units. The diff doesn't look too bad and as long as we tell users the expected units for each param we should be fine (they won't really see all the internal unit conversions anyway). I'll put up a PR so we can run tests on the branch |
…y annotations across MD integrators
|
@sek1ro-yuzzz could you test your SevenNet NaN issue on this branch please? |
… initialization in integrators
|
I tested this PR with this script and it passes on this branch, but fails on main |
|
thanks @curtischong! |
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: