{"action":"created","issue":{"url":"https://api.github.com/repos/pandegroup/openmm/issues/2076","repository_url":"https://api.github.com/repos/pandegroup/openmm","labels_url":"https://api.github.com/repos/pandegroup/openmm/issues/2076/labels{/name}","comments_url":"https://api.github.com/repos/pandegroup/openmm/issues/2076/comments","events_url":"https://api.github.com/repos/pandegroup/openmm/issues/2076/events","html_url":"https://github.com/pandegroup/openmm/pull/2076","id":324196615,"number":2076,"title":"Improved speed of applying patches","user":{"login":"peastman","id":4379786,"avatar_url":"https://avatars3.githubusercontent.com/u/4379786?v=4","gravatar_id":"","url":"https://api.github.com/users/peastman","html_url":"https://github.com/peastman","followers_url":"https://api.github.com/users/peastman/followers","following_url":"https://api.github.com/users/peastman/following{/other_user}","gists_url":"https://api.github.com/users/peastman/gists{/gist_id}","starred_url":"https://api.github.com/users/peastman/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/peastman/subscriptions","organizations_url":"https://api.github.com/users/peastman/orgs","repos_url":"https://api.github.com/users/peastman/repos","events_url":"https://api.github.com/users/peastman/events{/privacy}","received_events_url":"https://api.github.com/users/peastman/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":10,"created_at":"2018-05-17T21:42:46Z","updated_at":"2018-05-18T19:01:04Z","closed_at":"2018-05-18T17:14:04Z","author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/pandegroup/openmm/pulls/2076","html_url":"https://github.com/pandegroup/openmm/pull/2076","diff_url":"https://github.com/pandegroup/openmm/pull/2076.diff","patch_url":"https://github.com/pandegroup/openmm/pull/2076.patch"},"body":"When building a system with the CHARMM36 force field, this will often save about ten seconds.\r\n\r\nIt first tries to match every residue to a template without using patches.  If it succeeds, that's fine.  But if even one residue can't be matched, in then needs to build a list of all possible patched templates.  (And it really does need to build all of them, since it needs to throw an exception if there's more than one way to match it.)  So for every template, it needs to apply all possible combinations of patches.  In some cases, that led to thousands of patched versions of a single template.\r\n\r\nTo do that, it needs to decide which patches are compatible with each other.  Before it did that in a naive way: just try to apply each patch on top of the others, and if it throws an exception (for example, because a patch wants to modify an atom that another patch has already deleted), ignore that patch and continue.  But that approach was too forgiving and allowed combinations that really shouldn't be allowed.  For example, if two patches both modify the same atom, they should be considered incompatible since it's ambiguous what the parameters of that atom should be.\r\n\r\nSo now it records a list of every atom that a patch alters in any way (adds, changes, or deletes).  Two patches are considered incompatible if there's any atom they both alter in any way.  This reduces the set of allowed combinations and makes it a lot faster to build the patched templates.\r\n\r\nNow that it's doing that more rigorous check, it really shouldn't catch exceptions any more.  Any remaining exceptions indicate genuine mistakes in the force field definition.  But when I tried removing the exception handling, CHARMM36 produced an error!\r\n\r\n```\r\nValueError: Patch 'HS2' adds an atom with the same name as an existing atom: HE2\r\n```\r\n\r\nAnd sure enough, it's right.  The patch `HS2` adds an atom called `HE2`.  The residue templates `HSP` and `EIMM` each specify `<AllowPatch name=\"HS2\"/>` but already have an atom called `HE2`.  It looks like the logic for selecting allowed patches is including ones it shouldn't.  So we should fix that, but for the moment, I'll have ForceField continue to catch the exceptions so I don't break CHARMM36."},"comment":{"url":"https://api.github.com/repos/pandegroup/openmm/issues/comments/390302094","html_url":"https://github.com/pandegroup/openmm/pull/2076#issuecomment-390302094","issue_url":"https://api.github.com/repos/pandegroup/openmm/issues/2076","id":390302094,"user":{"login":"jchodera","id":3656088,"avatar_url":"https://avatars1.githubusercontent.com/u/3656088?v=4","gravatar_id":"","url":"https://api.github.com/users/jchodera","html_url":"https://github.com/jchodera","followers_url":"https://api.github.com/users/jchodera/followers","following_url":"https://api.github.com/users/jchodera/following{/other_user}","gists_url":"https://api.github.com/users/jchodera/gists{/gist_id}","starred_url":"https://api.github.com/users/jchodera/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jchodera/subscriptions","organizations_url":"https://api.github.com/users/jchodera/orgs","repos_url":"https://api.github.com/users/jchodera/repos","events_url":"https://api.github.com/users/jchodera/events{/privacy}","received_events_url":"https://api.github.com/users/jchodera/received_events","type":"User","site_admin":false},"created_at":"2018-05-18T19:01:04Z","updated_at":"2018-05-18T19:01:04Z","author_association":"CONTRIBUTOR","body":"> This particular problem is part of the forcefield conversion, and it's our job to do that right. \r\n\r\nI think the problem here is that none of us have the knowledge to make these decisions with 100% accuracy because the information we need is not contained in the parameter files at all. Even though there are other routes to loading in CHARMM systems, it seems problematic that we might accidentally omit a patch someone will want to use (or add). Can we at least provide some mechanism to allow people to specify patch overrides if they need to?"}}
{"actor":{"display_login":"jchodera"}}