{"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":7,"created_at":"2019-07-03T22:52:12Z","updated_at":"2019-07-04T04:41:09Z","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/508335417","html_url":"https://github.com/openforcefield/openforcefield/issues/374#issuecomment-508335417","issue_url":"https://api.github.com/repos/openforcefield/openforcefield/issues/374","id":508335417,"node_id":"MDEyOklzc3VlQ29tbWVudDUwODMzNTQxNw==","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},"created_at":"2019-07-04T04:41:09Z","updated_at":"2019-07-04T04:41:09Z","author_association":"NONE","body":"@davidlmobley Thanks for sharing your thoughts on the options. \r\n\r\n> Option (d) has always been out.\r\nSure as you said we can remove this to simplify the discussion.\r\n\r\n> It sounds like Yudong and @leeping 's biggest concern is allowing multiple units for the same type of force constants.\r\n\r\nI agree that the biggest concern is allowing multiple units for the same type of force constants. Option (c) will enforce that so it's better than (a) and (b) in my mind, but the serialization problem seems to be very hard to solve technically, so I guess we need to remove option (c).\r\n\r\nAmong option (a) and (b), option (b) is directly supported by ForceBalance, and option (a) can be supported by small tweaks at the code. The real concern is that the parameter of the same type can have values vary by many orders of magnitude, when different units are used. This could cause trouble, and I think it only brings a small benefit that it could save a few seconds from unit conversion for people who are already familiar with the `simtk.unit` package that are manually writing new terms into the file. (If people use the OFF toolkit API to modify the file, then unit conversion can be done before writing into the file.\r\n\r\n> Just throwing it out there... but would it be easier for you guys, @yudongqiu and @leeping , if the units had to be CONSISTENT within any section, though they remained attached to individual parameters? \r\n\r\nExactly, if the extra flexibility of \"allowing multiple units used for parameters of the same type\" is removed, then both (a) and (b) are good with me. I slightly favor option (b) as it's easier to parse in programs. \r\n\r\n> For example, we ought to be able to get dramatic speedups in assigning parameters to systems by simply implementing #51 and looking for SMIRKS matches in reverse order.\r\n\r\nI have already implemented hacks to cache the parameter assignments, so I guess this wouldn't improve my timings by much. Without the various hacks, it takes 10x longer and a single fitting will take months. The currently issue is the ForceField object creation using toolkit takes ~5x longer than before."}}
{"actor":{"display_login":"yudongqiu"}}