[gmx-developers] Virtual site live review tomorrow

[Pascal Merz]

Hi Erik,

I see that I formulated my email a bit imprecisely. I mentioned in the telco earlier today that I will spend some time before the live review to see if there are things that can be separated into separate commits (with future bisection in mind). But I’m convinced that it makes sense to review them together. Whether this ends up being several MRs or one MR with non-squashed commits is up for discussion.

Also, I am certainly willing to take responsibility for the change. I am reasonably confident to do so since before this change, we had almost no test coverage of virtual sites (except for a few regression tests) - at the level that some virtual site types were introduced, but could not be run because the rest of the infrastructure was never updated to allow for additional types. The change introduces unit and end-to-end tests, while not breaking any of the regression tests that were present. The current change also fixes several issues: The virtual site positions were slightly wrong in some cases, the virtual velocities ranged from totally wrong to slightly wrong to “correct by chance”.

I am fully aware that there might be additional issues, but I am also confident that I am leaving the code in significantly better shape than it was before.

Best,
Pascal



On Feb 10, 2021, at 10:32 AM, Erik Lindahl <erik.lindahl at gmail.com<mailto:erik.lindahl at gmail.com>> wrote:

Hi,

Unfortunately I have zero time the next two weeks.

My main worry with these large changes is not the first-time review (which indeed can be done with a live review), but that we've had several such large changes cause subtle bugs with potentially devastating results for other simulations - and apart from the fact that it's take a while to notice them, it's also taken a while to find when it's something that was part of a large change.

In particular the ~700 changed lines in vsite.cpp appears to be a mix of general cleanup and a couple of different changes - so I don't quite see how it's *impossible* to separate those into something more manageable.

So, just remember that it's not merely a matter of believing the code path you focus on works, but that the ~700 lines won't have side effects on any other code paths, and that large changes going in also means writing a blank check to rapidly look into any future bugs that are bisected to that change ;-)

Cheers,

Erik


On Wed, Feb 10, 2021 at 6:12 PM Pascal Merz <Pascal.Merz at colorado.edu<mailto:Pascal.Merz at colorado.edu>> wrote:
Hi all,


The virtual sites MR at https://gitlab.com/gromacs/gromacs/-/merge_requests/979 is rather large, but cannot easily be split into smaller MRs. It should be reasonably easy to review with a bit of guidance, however, since the large majority of line changes are documentation, tests, and repetitive changes.

To that end, Mark and I will have a live review session of the change on Feb 11, 9am MST / 5pm CET. If someone else would like to jump on to help the review, I would very much appreciate it! I think that we should be able to keep it around 30 minutes, but it all depends on how many issues we find :) We’ll meet at https://cuboulder.zoom.us/j/94868986659.

Thank you,
Pascal

--
Gromacs Developers mailing list

* Please search the archive at http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List before posting!

* Can't post? Read http://www.gromacs.org/Support/Mailing_Lists

* For (un)subscribe requests visit
https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers or send a mail to gmx-developers-request at gromacs.org<mailto:gmx-developers-request at gromacs.org>.


--
Erik Lindahl <erik.lindahl at dbb.su.se<mailto:erik.lindahl at dbb.su.se>>
Professor of Biophysics, Dept. Biochemistry & Biophysics, Stockholm University
Science for Life Laboratory, Box 1031, 17121 Solna, Sweden
--
Gromacs Developers mailing list

* Please search the archive at http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List before posting!

* Can't post? Read http://www.gromacs.org/Support/Mailing_Lists

* For (un)subscribe requests visit
https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers or send a mail to gmx-developers-request at gromacs.org<mailto:gmx-developers-request at gromacs.org>.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://maillist.sys.kth.se/pipermail/gromacs.org_gmx-developers/attachments/20210210/acc56364/attachment-0001.html>