From: Grace Brannigan (grace.brannigan_at_rutgers.edu)
Date: Mon Jun 27 2016 - 06:41:33 CDT
Hi Brian,
On Wed, Jun 22, 2016 at 6:22 PM, Brian Radak <bradak_at_anl.gov> wrote:
>
> Fortunately, although I don't think I outlined this well enough, the error
> was actually quite small in its extent, since the core simulation dynamics
> (i.e. the forces and energies) were still correct (that's what my error
> testing did verify!). The only bug (now fixed) was in the FEP *analysis* code.
> Post processing of the trajectories would have lead to the expected result
> (I should also note that multistate techniques such as WHAM are now quite
> standard in the field).
Yes, I did understand that. We certainly don't rely exclusively on the namd
analysis, but there are many reasons we find it convenient at times - and
though the error may have been small in 'extent' it was unusually large in
magnitude, making it seem like a bug particularly deserving of a rapid
fix.
But I was mainly using it as an example of a larger issue regarding
untested updates - i.e. the fact that such an obvious and trivially fixed
error made it into the development version is alarming, because it
highlights the incomplete testing process.
Again, not to minimize the mistake, but the behavior before the bug was
> actually quite a bit more pernicious in my opinion, because alchemical
> scaling of bonded terms was entirely excluded and could (at least
> heuristically) lead to unsatisfying behavior in certain circumstances.
> There was, however, no indication from the code output that this kind of
> situation was possible.
Well, for users who scale bonded terms, this update might have been a net
positive - but for users who decouple ligands and don't scale bonded terms,
it was, if anything, a net negative. Very easy to work around, sure, but I
just want to make sure that this (pretty common) type of usage is
considered when changes are committed to the development version, before
something more insidious creeps in.
>
>
> Even undocumented fixes can introduce unexpected discrepancies compared to
> previous results, which can take a user some time to track down if they
> don't know where to look. You might know it's an improvement - but the
> user doesn't.
>
> This is true and of course to be avoided. Unfortunately undocumented
> features aren't always even apparent to all of the developers! Such are the
> perils of academic software.
>
> Jerome and I spoke for a while about the situation you inherited, when he
visited after the developers conference in Chicago, and I really am
sympathetic - although I also think the alchemy code's chaotic history
demands even more attention to documentation and testing prior to
committing changes, rather than less. As a user, in any case, a
conservative approach to committing changes would be appreciated.
>
> So, I'd like to request that you be considerate to the numerous research
> groups using the FEP features when choosing to commit changes. And let me
> know if you need some unusual test cases...
>
> Duly noted. Candidates for regression testing would be quite welcome, as
> this is something that we need to move towards in order to prevent problems
> in the future. A community based effort could go a long way in making this
> happen and further boost the other ongoing developments to the code base.
>
Yes, this seems like the only way forward. I hadn't realized regression
tests weren't being done previously - although I guess that also means even
rudimentary tests would be an improvement.
The simplest one that comes to mind is just comparison of deltaG reported
in the .fepout for decoupling of a small molecule from water. These should
probably be run for at least a few ligands with a range of solubilities,
although any ligand would have revealed this most recent bug...Or maybe you
did run these but just analyzed with something fancier? More generally, is
there any reason to expect any of your updates would affect the calculated
solvation free energy?
Thanks again for your efforts toward improving the alchemy features - I've
been voicing concern about committing improvements prematurely, but I
should clarify that concern comes from an appreciation of how easy it is
for problems to develop, rather than an expectation that development should
be straightforward.
Grace
> Regards,
> Brian
>
>
>
> Thanks,
>
> --
> Grace Brannigan, Ph.D.
> Assistant Professor
> Center for Computational and Integrative Biology (CCIB) &
> Department of Physics
> Rutgers University, Camden, NJ
> (856)225-6780
> http://branniganlab.wordpress.com
>
>
>
On Wed, Jun 22, 2016 at 6:22 PM, Brian Radak <bradak_at_anl.gov> wrote:
>
>
> On 06/21/2016 01:03 PM, Grace Brannigan wrote:
>
> Hi Brian,
>
> As a decade-long user of the alchemy code, I certainly welcome
> improvements to consistency and accuracy, and appreciate you working to
> implement fixes.
>
> It is alarming, though, to hear that changes are being committed casually
> and without documentation. A few months ago, for instance, my postdoc Reza
> Salari observed discrepancies of ~30,000 kcal/mol in FEP output from the
> development version. That was hard to miss, but naturally makes us wonder
> about less obvious bugs.
>
> Fortunately, although I don't think I outlined this well enough, the error
> was actually quite small in its extent, since the core simulation dynamics
> (i.e. the forces and energies) were still correct (that's what my error
> testing did verify!). The only bug (now fixed) was in the FEP *analysis*
> code. Post processing of the trajectories would have lead to the expected
> result (I should also note that multistate techniques such as WHAM are now
> quite standard in the field).
>
> Again, not to minimize the mistake, but the behavior before the bug was
> actually quite a bit more pernicious in my opinion, because alchemical
> scaling of bonded terms was entirely excluded and could (at least
> heuristically) lead to unsatisfying behavior in certain circumstances.
> There was, however, no indication from the code output that this kind of
> situation was possible.
>
>
>
> Even undocumented fixes can introduce unexpected discrepancies compared to
> previous results, which can take a user some time to track down if they
> don't know where to look. You might know it's an improvement - but the
> user doesn't.
>
> This is true and of course to be avoided. Unfortunately undocumented
> features aren't always even apparent to all of the developers! Such are the
> perils of academic software.
>
>
>
> So, I'd like to request that you be considerate to the numerous research
> groups using the FEP features when choosing to commit changes. And let me
> know if you need some unusual test cases...
>
> Duly noted. Candidates for regression testing would be quite welcome, as
> this is something that we need to move towards in order to prevent problems
> in the future. A community based effort could go a long way in making this
> happen and further boost the other ongoing developments to the code base.
>
> Regards,
> Brian
>
>
>
> Thanks,
>
> --
> Grace Brannigan, Ph.D.
> Assistant Professor
> Center for Computational and Integrative Biology (CCIB) &
> Department of Physics
> Rutgers University, Camden, NJ
> (856)225-6780
> http://branniganlab.wordpress.com
>
>
>
>
>
>
>
> On Tue, Jun 21, 2016 at 10:48 AM, Jérôme Hénin <jerome.henin_at_ibpc.fr>
> wrote:
>
>> Brian, thanks for outlining those issues, in particular missing docs. I
>> can't recommend enough that the docs be updated at the same time as the
>> features themselves. It's vital for users of development versions, and it's
>> the best time to do it as a developer as well - not least, it removes the
>> risk that some things never get documented at all, like the WCA features--94eb2c03634ae84f790536410287--
This archive was generated by hypermail 2.1.6 : Tue Dec 27 2016 - 23:22:17 CST