payload: {"action":"created","issue":{"url":"https://api.github.com/repos/openforcefield/openforcefield/issues/374","repository_url":"https://api.github.com/repos/openforcefield/openforcefield","labels_url":"https://api.github.com/repos/openforcefield/openforcefield/issues/374/labels{/name}","comments_url":"https://api.github.com/repos/openforcefield/openforcefield/issues/374/comments","events_url":"https://api.github.com/repos/openforcefield/openforcefield/issues/374/events","html_url":"https://github.com/openforcefield/openforcefield/issues/374","id":463985442,"node_id":"MDU6SXNzdWU0NjM5ODU0NDI=","number":374,"title":"Discussion about explicit units for each parameter in SMIRNOFF version 0.3","user":{"login":"yudongqiu","id":6952367,"node_id":"MDQ6VXNlcjY5NTIzNjc=","avatar_url":"https://avatars0.githubusercontent.com/u/6952367?v=4","gravatar_id":"","url":"https://api.github.com/users/yudongqiu","html_url":"https://github.com/yudongqiu","followers_url":"https://api.github.com/users/yudongqiu/followers","following_url":"https://api.github.com/users/yudongqiu/following{/other_user}","gists_url":"https://api.github.com/users/yudongqiu/gists{/gist_id}","starred_url":"https://api.github.com/users/yudongqiu/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/yudongqiu/subscriptions","organizations_url":"https://api.github.com/users/yudongqiu/orgs","repos_url":"https://api.github.com/users/yudongqiu/repos","events_url":"https://api.github.com/users/yudongqiu/events{/privacy}","received_events_url":"https://api.github.com/users/yudongqiu/received_events","type":"User","site_admin":false},"labels":[{"id":574114611,"node_id":"MDU6TGFiZWw1NzQxMTQ2MTE=","url":"https://api.github.com/repos/openforcefield/openforcefield/labels/question","name":"question","color":"cc317c","default":true},{"id":1311581635,"node_id":"MDU6TGFiZWwxMzExNTgxNjM1","url":"https://api.github.com/repos/openforcefield/openforcefield/labels/under%20discussion","name":"under discussion","color":"9fc2f9","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":16,"created_at":"2019-07-03T22:52:12Z","updated_at":"2019-07-04T17:53:42Z","closed_at":null,"author_association":"NONE","body":"A while ago a discussion about API change leads to the adding of physical units explicitly for each parameter. #291\r\n\r\nThis leads to a new feature where parameters belongs to the same force field type can have different units, i.e., this is now supported by the openforcefield toolkit:\r\n```\r\n<Bond smirks=\"[#6X4:1]-[#6X4:2]\" length=\"1.526 * angstrom\" k=\"620.0 * angstrom**-2 * mole**-1 * kilocalorie\" id=\"b1\"></Bond>\r\n<Bond smirks=\"[#6X4:1]-[#6X3:2]\" length=\"0.151 * nanometer\" k=\"265265.6 * nanometer**-2 * mole**-1 * kilojoule\" id=\"b2\"></Bond>\r\n```\r\n\r\nAfter discussion with @leeping, I think this extra flexibility of allowing multiple units for the same type of force constants is a bit unnecessary, and creates additional complications for parsing and converting the file. After a discussion with @j-wags, we think it's worth opening this up for discussion, so we can reach a consensus. Here, we provide some candidates for alternative formats for discussion and vote. The candidates are:\r\n\r\n- (a) `<Bond smirks=\"[#6X4:1]-[#6X4:2]\" length=\"1.526 * angstrom\" .. />`\r\n    This is currently used in SMIRNOFF v0.3\r\n\r\n- (b) `<Bond smirks=\"[#6X4:1]-[#6X4:2]\" length=\"1.526\" length_unit=\"angstrom\" .. />`\r\n    This provides almost the same flexibility as (a) but allows easier parsing the xml file.\r\n\r\n- (c) `<Bonds length_unit=\"angstrom\" .. />`\r\n    The unit is defined in the parent element together with force type. This was used in SMIRNOFF v0.2\r\n\r\n- (d) `<Bond smirks=\"[#6X4:1]-[#6X4:2]\" length=\"1.526\" .. />`\r\n    The default unit is fixed to be consistent with the default units in OpenMM.\r\n\r\n\r\nA few points that may be worth considering:\r\n- @j-wags mentioned options (a), (b), and (d) support the direct serialization of each term in the forcefield, but (c) does not.\r\n- [option (d)] OpenMM and other existing simulation packages all define a single default unit that they use for their internal data. Experts in the field are most accustomed to this model. The responsibility to provide \"correct\" parameter values w.r.t. unit conversion is therefore on the force field writer, instead of the toolkit.\r\n- [option (a)] The version 0.3 spec's units-with-values parameter format requires expensive parsing of each entry, which is computationally inefficient, and adds up to a significant fraction of ForceBalance's current runtime.\r\n- New users may not be familiar with the `simtk.unit` package, and it is hard to find documentation of which units are supported.\r\n- ForceBalance parses the offxml file in a similar way to the OpenMM xml file, and currently supports options (b), (c) and (d). It creates extra complexity if different explicit units are used, because the user needs to define the \"prior width\", indicating the magnitude of parameter change during fitting. The \"prior width\" are expected to be defined for a type of parameter that uses the same unit."},"comment":{"url":"https://api.github.com/repos/openforcefield/openforcefield/issues/comments/508548340","html_url":"https://github.com/openforcefield/openforcefield/issues/374#issuecomment-508548340","issue_url":"https://api.github.com/repos/openforcefield/openforcefield/issues/374","id":508548340,"node_id":"MDEyOklzc3VlQ29tbWVudDUwODU0ODM0MA==","user":{"login":"leeping","id":1441560,"node_id":"MDQ6VXNlcjE0NDE1NjA=","avatar_url":"https://avatars1.githubusercontent.com/u/1441560?v=4","gravatar_id":"","url":"https://api.github.com/users/leeping","html_url":"https://github.com/leeping","followers_url":"https://api.github.com/users/leeping/followers","following_url":"https://api.github.com/users/leeping/following{/other_user}","gists_url":"https://api.github.com/users/leeping/gists{/gist_id}","starred_url":"https://api.github.com/users/leeping/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/leeping/subscriptions","organizations_url":"https://api.github.com/users/leeping/orgs","repos_url":"https://api.github.com/users/leeping/repos","events_url":"https://api.github.com/users/leeping/events{/privacy}","received_events_url":"https://api.github.com/users/leeping/received_events","type":"User","site_admin":false},"created_at":"2019-07-04T17:53:42Z","updated_at":"2019-07-04T17:53:42Z","author_association":"CONTRIBUTOR","body":"I agree with everything you said above and I'm okay with a compromise if needed.  Ideally I'd like to see support for option (c) which would allow unit definitions and other common properties in the header. My viewpoint is more \"file-centric\" because FB has always manipulated force fields at the file level.  If we require explicit units for each parameter and the toolkit checks that they're all the same, I'm okay with that too, but we should be aware of the future issues it might cause.  \r\n\r\nI'd mainly like to avoid a situation where all \"common properties\", including units, need to be defined at the individual parameter level.  The other common property that I saw was `idivf` which is an integer, but other force types that we might want for the future could have larger numbers of common properties."}}
other: {"actor":{"display_login":"leeping"}}