{"action":"created","issue":{"url":"https://api.github.com/repos/michellab/Sire/issues/192","repository_url":"https://api.github.com/repos/michellab/Sire","labels_url":"https://api.github.com/repos/michellab/Sire/issues/192/labels{/name}","comments_url":"https://api.github.com/repos/michellab/Sire/issues/192/comments","events_url":"https://api.github.com/repos/michellab/Sire/issues/192/events","html_url":"https://github.com/michellab/Sire/issues/192","id":296848249,"number":192,"title":"AmberDihedral unable to expand series with negative k values.","user":{"login":"lohedges","id":648557,"avatar_url":"https://avatars1.githubusercontent.com/u/648557?v=4","gravatar_id":"","url":"https://api.github.com/users/lohedges","html_url":"https://github.com/lohedges","followers_url":"https://api.github.com/users/lohedges/followers","following_url":"https://api.github.com/users/lohedges/following{/other_user}","gists_url":"https://api.github.com/users/lohedges/gists{/gist_id}","starred_url":"https://api.github.com/users/lohedges/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lohedges/subscriptions","organizations_url":"https://api.github.com/users/lohedges/orgs","repos_url":"https://api.github.com/users/lohedges/repos","events_url":"https://api.github.com/users/lohedges/events{/privacy}","received_events_url":"https://api.github.com/users/lohedges/received_events","type":"User","site_admin":false},"labels":[{"id":194158343,"url":"https://api.github.com/repos/michellab/Sire/labels/bug","name":"bug","color":"fc2929","default":true}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":7,"created_at":"2018-02-13T18:57:20Z","updated_at":"2018-02-17T16:33:20Z","closed_at":null,"author_association":"CONTRIBUTOR","body":"It appears that the `AmberDihedral` object cannot expand series of the form `k [ 1 + cos (per Phi - phase) ]` when there are terms with negative values for `k`. For example, consider the following series (taken from an actual CHARMM dihedral function),\r\n\r\n```\r\nfunc = 0.3 [ 1 + cos Phi ]  - 0.3 [ 1 + cos (4 Phi) ],\r\n```\r\n\r\nwhich is expressed by the following by Sire\r\n\r\n```\r\nfunc = 0.3  cos Phi  - 0.3 cos (4 Phi) ].\r\n```\r\nAlthough this is a valid series, attempting to convert this to an `AmberDihedral` fails:\r\n\r\n```python\r\nfrom Sire.CAS import *\r\nfrom Sire.MM import *\r\n\r\n# Create the function expression.\r\nfunc = Expression(0.3 * Cos(Symbol(\"Phi\")) - 0.3 * Cos(4 * Symbol(\"Phi\")))\r\n\r\n# Attempt to create an AmberDihedral object.\r\namber_dihedral = AmberDihedral(func, Symbol(\"Phi\"))\r\n```\r\n\r\nNow assume that the terms have the same sign for `k`, i.e.:\r\n\r\n```\r\nfunc = 0.3 [1 + cos Phi ] + 0.3 [1 + cos (4 Phi) ]\r\n     = 0.6 + 0.3 cos Phi + 0.3 cos (4 Phi)\r\n```\r\n(I know this can be simplified further, I'm just expressing it in the same way as Sire.)\r\n\r\nThe following works fine:\r\n\r\n```python\r\nfunc = Expression(0.6 + 0.3 * Cos(Symbol(\"Phi\")) + 0.3 * Cos(4 * Symbol(\"Phi\")))\r\namber_dihedral = AmberDihedral(func, Symbol(\"Phi\"))\r\n```\r\nIt seems that any negative `k ` values fails, e.g. a single term:\r\n\r\n```python\r\nfunc = Expression(-0.3 * (1 + Cos(Symbol(\"Phi\"))))\r\namber_dihedral = AmberDihedral(func, Symbol(\"Phi\"))\r\n```\r\nPerhaps negative `k` values (repulsive terms) are non-standard, although they seem to appear in the CHARMM parameter files (albeit extremely rarely).\r\n\r\nI'll see if I can modify the `AmberDihedral` class to handle these terms."},"comment":{"url":"https://api.github.com/repos/michellab/Sire/issues/comments/366452997","html_url":"https://github.com/michellab/Sire/issues/192#issuecomment-366452997","issue_url":"https://api.github.com/repos/michellab/Sire/issues/192","id":366452997,"user":{"login":"lohedges","id":648557,"avatar_url":"https://avatars1.githubusercontent.com/u/648557?v=4","gravatar_id":"","url":"https://api.github.com/users/lohedges","html_url":"https://github.com/lohedges","followers_url":"https://api.github.com/users/lohedges/followers","following_url":"https://api.github.com/users/lohedges/following{/other_user}","gists_url":"https://api.github.com/users/lohedges/gists{/gist_id}","starred_url":"https://api.github.com/users/lohedges/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lohedges/subscriptions","organizations_url":"https://api.github.com/users/lohedges/orgs","repos_url":"https://api.github.com/users/lohedges/repos","events_url":"https://api.github.com/users/lohedges/events{/privacy}","received_events_url":"https://api.github.com/users/lohedges/received_events","type":"User","site_admin":false},"created_at":"2018-02-17T16:33:20Z","updated_at":"2018-02-17T16:33:20Z","author_association":"CONTRIBUTOR","body":"@chryswoods I totally agree with what you're saying. When used as it was before (with no additional argument to the constructor) the AmberDihedral class should only hold valid Amber dihedrals, since those are what are supported by Amber.\r\n\r\nThe point is that the existing code in devel fails to create an AmberDihedral if you pass in a cosine series with any negative k values. While the constructor adds a phase shift when detecting negative k, it doesn't correct for the swapped sign of k when comparing the sum of the constant terms, so gives an error. This is what I was seeing when I was attempting to represent certain CHARMM dihedrals using an AmberDihedral object. My changes should fix this problem, i.e. you always create a valid AmberDihedral (no negative k values) regardless of the signs in the cosine series expression that you pass in, which is what we want.\r\n\r\nThese changes shouldn't break anything in the existing code and I've re-run all the tests to check. The optional boolean flag just means that you can keep the negative k representation if desired, as I do with the CHARMM dihedrals (although it's probably fine to just use the phase shifted version). There are no other changes to the API.\r\n\r\nAs mentioned above, the code below now works, whereas in devel I get an error when creating a dihedral. Note that the AmberDihedral object is valid, i.e. k is always positive.\r\n\r\n```python\r\nfrom Sire.CAS import *\r\nfrom Sire.MM import *\r\n\r\nPhi = Symbol(\"Phi\")\r\n\r\n# Create a two-term dihedral expression with different signs.\r\nfunc = Expression(0.3 * (1 + Cos(Phi)) - 0.7 * (1 + Cos(4 * Phi)))\r\n\r\n# Create an AMBER style dihedral (with positive k values).\r\namber_dihedral = AmberDihedral(func, Phi)\r\n\r\n# amber_dihedral.terms =\r\n# <bound method terms of AmberDihedral( k[0] = 0.3, periodicity[0] = 1, phase[0] = 0, \r\n# k[1] = 0.7, periodicity[1] = 4, phase[1] = -3.14159 )>\r\n```\r\nSorry if this wasn't clear.\r\n\r\nThanks for the info on making comparisons between Sire.CAS expressions. I assumed that this was something that was pretty tricky in general and beyond the scope of what's needed. After all, we're not writing Mathematica ;-)\r\n\r\nCheers."}}
{"actor":{"display_login":"lohedges"}}