> > Yes we should go back to the original approach that I put implemented
> when
> > I
> > added it to Sander and PMEMD and had Wei put into Sleap. Why was this
> > changed??? Where was the rational and the discussion for changing it
> on the
> > AMBER dev mailing list?
> >
>
> I'm guessing here a failure to communicate. I freaked out when I first
> saw
> 0s there, since I didn't see why those would ever be present. Nowhere
> did
> I actually see a note about why there were 0 terms in the SCEE/SCNB
> scale
> factors, so it's reasonable that they would be removed by those that
> didn't
> know the idea behind them.
I won't name names here but the following is the process by which this was
implemented.
Certain people requested this feature desperately in sander and pmemd so
they could correctly run mixed FF99SB / Glycam06 simulations. I added this
to Sander and PMEMD and then provided a template for how the prmtop should
look, including the note about making things zero for the 1-4 terms that
should not be calculated. I then handed this over to those that wanted the
functionality and asked them to extensively test it. This was then passed
onto Wei to be implemented in sleap and ultimately to also be added to
t/xleap.
I created test cases for sander and PMEMD to verify that things were working
and I manually carried out calculations by hand (yes with pencil and paper)
to verify that things were correct. I also updated the following page:
http://ambermd.org/formats.html
%FLAG SCEE_SCALE_FACTOR
%FORMAT(5E16.8) (ONE_SCEE(i), i=1,NPTRA)
ONE_SCEE : 1-4 electrostatic scaling constant. It is inverted right after
it's read in for performance reasons. This allows variable
1-4 scaling. If not present, it defaults to 1.2 for all
dihedrals.
Therefore, the default ONE_SCEE value in the code is 1.0/1.2
%FLAG SCNB_SCALE_FACTOR
%FORMAT(5E16.8) (ONE_SCNB(i), i=1,NPTRA)
ONE_SCNB : 1-4 VDW scaling constant. It is inverted right after
it's read in. This allows variable 1-4 scaling. If not present,
it defaults to 2.0 for all dihedrals. Therefore, the default
ONE_SCNB
value in the code is 1.0/2.0
It was then left to others to decide how the force field dat files etc
should be updated and this implemented in t/xleap. At this point the updates
should have been documented and t/xleap test cases created by those that
added that functionality to the code. Sadly it appears that this was not
done.
> >From the commit logs, it appears as though there were some conflicts
> resolved by people modifying the same parts separately (happens all the
> time, and git makes this not nearly as bad to deal with). To be fair,
> a
> feature important enough to remain invariant should be documented
> outside
> of mailing list archives ;).
Exactly! This code should have been checked into the tree as it was being
written, test cases should have been created and documentation added. This
was not done and instead we get a bunch of people working on the same code
to do different things and then someone else has to integrate them all. In
both cases with no test cases or documentation. The various people writing
each section of the code should have taken responsibility for getting their
piece documented, tested and added to the tree.
> No explicit test cases had been written for leap since the advent of
> the
> new (10-year-old) format, so we have a *lot* of catching up to do.
Yes but this is of course NO excuse for not adding new test cases. My point
is the person who added new functionality to leap should have stopped and
thought... "I need to be careful not to break the existing code here." Then
they would say "Oh let me see what the test cases cover..." - "Oh I see
nobody ever created test cases in the first place. I better do that before I
get started." Then they create the test cases and assume AMBER 11 is the
current gold standard - maybe check against AMBER 10 as well just to see if
there is some variation. Create the test cases and add them to the tree.
Then they add their new code, create test cases for that and then make sure
both their new code works and, more importantly, their new code does not
break the test cases they created before they started.
What is the problem with this? - Is it a lack of guidance? - In which case
we should probably have discussion amongst the PIs and supervisors how best
to instill this in our postdocs and students. Is it laziness? I hope not. Is
it massive overwork and pressure to produce? - I suspect a combination of
the later and a lack of guidance. Unfortunately when working on such a large
integrated project, where thousands of people will ultimately rely on the
code, it is vital that we all take some responsibility to check and improve
things. Given the overwork and pressure to produce it is not appropriate for
any of us to just assume someone else (who is also overworked and under
similar pressures) will come along and fix things for us. It would be
awesome if we had a couple of full time professional programmers working on
AMBER that could take care of this for us but until one of us makes millions
and agrees to pay for such people or NIH has some epiphany and gives us a
ton of money for this it is not going to happen so we have to make do with
what we have.
> While
> we certainly need to update *what* we test in tleap here, it's hardly
> just
> the people of today's fault for our situation. The existing tests were
> so
> incomplete that it never informed editors that they actually changed
> existing behavior.
Agreed. But the people adding to the code now should have seen that the
first thing they needed to do for their project was to set a start point
which would mean creating an initial set of test cases if they didn't
already exist.
> To make up for lost time, though, I think we have to compare AmberTools
> 1.5
> vanilla (or older?) output with the output from today *and validate*
> any
> changes that have occurred (for instance -- does anybody know if the
> difference incurred by the variable 1-4 scaling addition is really just
> a
> rearrangement of dihedral pointers? Was that expected?)
Agreed!
> P.S. To hop on the bandwagon with Dave here, I think what would
> *really*
> have avoided all of this in the first place is some _developer_
> documentation. This would be the ideal place to put stuff like "keep
> improper and multi-term dihedral scaling factors 0 because of -blah-",
> or
> "We currently test for -blah-". It's hard enough to go looking through
Yes this information with the explanation was sent to the people who wanted
this feature and were adding it to the code in a workable fashion. For some
reason the code was written and added but the test cases and documentation
were never added. I assume this must have been a simple oversight and in
fact it was all written and a simple 'git add', 'git commit', 'git push'
will solve the issue.
All the best
Ross
/\
\/
|\oss Walker
---------------------------------------------------------
| Assistant Research Professor |
| San Diego Supercomputer Center |
| Adjunct Assistant Professor |
| Dept. of Chemistry and Biochemistry |
| University of California San Diego |
| NVIDIA Fellow |
|
http://www.rosswalker.co.uk |
http://www.wmd-lab.org/ |
| Tel: +1 858 822 0854 | EMail:- ross.rosswalker.co.uk |
---------------------------------------------------------
Note: Electronic Mail is not secure, has no guarantee of delivery, may not
be read every day, and should not be used for urgent or sensitive issues.
_______________________________________________
AMBER-Developers mailing list
AMBER-Developers.ambermd.org
http://lists.ambermd.org/mailman/listinfo/amber-developers
Received on Sat Nov 12 2011 - 14:30:03 PST
