search

TinkerTools / tinker

Tinker: Software Tools for Molecular Design
https://dasher.wustl.edu/tinker/
Other
129 stars 61 forks source link

bug fix in charge flux chain rule term #78

Closed leucinw closed 3 years ago

leucinw commented 3 years ago

Hi @jayponder , the current implementation of bndcflux priority is only in the alterchg.f, where the value could be 1, 0, and -1 for a bond. It turns out this should also be in chain rule term (dcflux.f). Fortunately, for water, it is coincidently correct because we have water coordinate in O H H order., where the priority value of two O-H bonds is 1.

This will fix other molecules and H-H-O water.

zhi-wang commented 3 years ago

@roseane-reis

jayponder commented 3 years ago

Hi Chengwen, Thanks for noticing this. You are right, and your fix looks correct. But instead of doing it as you suggest, I think it's better to store the priority for each bond during the single call to "kchgflx". Then we won't have to keep recomputing the priority values every time we call "alterchg" or "dcflux", which are called every energy or gradient evaluation. I'll fix this by storing the priority values, and then probably close this pull request once I've got the new code pushed to Github. Does that sound OK?

leucinw commented 3 years ago

Hi Jay, I totally agree. You can close this issue.

On Sat, Jan 16, 2021, 11:29 Jay Ponder notifications@github.com wrote:

Hi Chengwen, Thanks for noticing this. You are right, and your fix looks correct. But instead of doing it as you suggest, I think it's better to store the priority for each bond during the single call to "alterchg". Then we won't have to keep recomputing the priority values every time we call "dcflux", which is called every minimization or MD step. I'll fix this by storing the priority values, and then probably close this pull request once I've got the new code pushed to Github. Does that sound OK?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/TinkerTools/tinker/pull/78#issuecomment-761601700, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHI63NMEMGERIDGYMKRAKUTS2HEGTANCNFSM4WE765LQ .

jayponder commented 3 years ago

This has been fixed, but by a different mechanism, in Tinker. The code now takes the first atom listed in each bond charge flux parameter as the "first priority", and the second atom listed as the "second priority". Thus the checking of priority within the chain rule term, as in the original post, is no longer needed. So I'm closing this pull request.