Hi Dave,
Thanks for doing the code documentation. This will prove very helpful down the line. Modifying the whitespace though will potentially making patching a pain especially the TI code update. The beta is already built against AMBER 16 and, fortunately we have merged everything into master already. Additional betas will be a pain in the butt though since we'll have to maintain 2 versions of the code, one with the new whitespace and one that matches AMBER 16. We may end up having to provide tar files of the entire CUDA directory instead of a patch for future betas which is not ideal (it precludes us making the beta public) but that's something we'll have to figure out.
Dan, Charlie - make sure you have all your latest changes merged into the master. Dave please work closely with Dan and Charlie to make sure this goes smoothly.
We should also do a sanity check on performance and code correctness although from what you describe this should only be cosmetic changes yes?
What do you have planned beyond the code review? We should coordinate things closely on this to make sure it doesn't conflict with what we plan running up to AMBER 18.
All the best
Ross
> On Apr 17, 2017, at 6:00 PM, David Cerutti <dscerutti.gmail.com> wrote:
>
> To all:
>
> I am nearly finished with my code review of pmemd.cuda. Comments have been
> added throughout with the goal of identifying every function's formal
> arguments, clarifying the beginning and end of various long blocks of code,
> and better documenting pre-processor directives with a long reach (i.e.
> this #endif ends what?).
>
> While I have tried to preserve Scott's convention of extending assignment
> statements to collect related variables into groups, a great deal of white
> space has been removed in the interest of adhering to a strict 96-character
> limit on each line. This will significantly increase the amount of code
> that can fit on the screen at one time, and permit two or more terminal
> windows to display separate libraries side by side for easy
> cross-referencing. Spacing has also been added or removed to emphasize the
> binding order of operations (I have a pretty detailed method, but please
> don't feel that it is required for future commits). Additionally, I have
> made an effort to decouple braces from pre-processor directives and provide
> indentation for the pre-processor directives themselves to help developers
> more easily gain a sense of where their code sits in very involved sections
> like the non-bonded inner loops in kNLCPNE.h.
>
> IF YOU HAVE OUTSTANDING EDITS and need to commit them, I have made no
> functional changes to the code quite yet--I wanted to get the review done
> before I start making other changes. Please send me a mail directly if you
> need to merge your items--it is straightforward to go back to the last
> commit from two weeks ago and see the code as it was beforehand to make a
> diff, but with the review nearly done I will help work out the best way to
> do the merge.
>
> Cheers,
> Dave
> _______________________________________________
> AMBER-Developers mailing list
> AMBER-Developers.ambermd.org
> http://lists.ambermd.org/mailman/listinfo/amber-developers
_______________________________________________
AMBER-Developers mailing list
AMBER-Developers.ambermd.org
http://lists.ambermd.org/mailman/listinfo/amber-developers
Received on Mon Apr 17 2017 - 15:30:04 PDT
