<div dir="ltr">I plan to attend, but may be a few minutes late.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Feb 10, 2021 at 8:01 PM Pascal Merz &lt;<a href="mailto:Pascal.Merz@colorado.edu">Pascal.Merz@colorado.edu</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">



<div style="overflow-wrap: break-word;">
Hi Erik,
<div><br>
</div>
<div>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.<br>
<div><br>
</div>
<div>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”.</div>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>Best,</div>
<div>Pascal<br>
<div><br>
<br>
</div>
<div><br>
<blockquote type="cite">
<div>On Feb 10, 2021, at 10:32 AM, Erik Lindahl &lt;<a href="mailto:erik.lindahl@gmail.com" target="_blank">erik.lindahl@gmail.com</a>&gt; wrote:</div>
<br>
<div>
<div dir="ltr">Hi,<br>
<div><br>
</div>
<div>Unfortunately I have zero time the next two weeks.</div>
<div><br>
</div>
<div>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&#39;ve had several such large changes cause subtle bugs with potentially devastating results for other simulations - and
 apart from the fact that it&#39;s take a while to notice them, it&#39;s also taken a while to find when it&#39;s something that was part of a large change.</div>
<div><br>
</div>
<div>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&#39;t quite see how it&#39;s *impossible* to separate those into something more manageable.  </div>
<div><br>
</div>
<div>So, just remember that it&#39;s not merely a matter of believing the code path you focus on works, but that the ~700 lines won&#39;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 ;-)</div>
<div><br>
</div>
<div>Cheers,</div>
<div><br>
</div>
<div>Erik</div>
<div><br>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Wed, Feb 10, 2021 at 6:12 PM Pascal Merz &lt;<a href="mailto:Pascal.Merz@colorado.edu" target="_blank">Pascal.Merz@colorado.edu</a>&gt; wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Hi all,<br>
<br>
<br>
The virtual sites MR at <a href="https://gitlab.com/gromacs/gromacs/-/merge_requests/979" rel="noreferrer" target="_blank">
https://gitlab.com/gromacs/gromacs/-/merge_requests/979</a> 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.<br>
<br>
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 <a href="https://cuboulder.zoom.us/j/94868986659" rel="noreferrer" target="_blank">
https://cuboulder.zoom.us/j/94868986659</a>.<br>
<br>
Thank you,<br>
Pascal<br>
<br>
-- <br>
Gromacs Developers mailing list<br>
<br>
* Please search the archive at <a href="http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List" rel="noreferrer" target="_blank">
http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List</a> before posting!<br>
<br>
* Can&#39;t post? Read <a href="http://www.gromacs.org/Support/Mailing_Lists" rel="noreferrer" target="_blank">
http://www.gromacs.org/Support/Mailing_Lists</a><br>
<br>
* For (un)subscribe requests visit<br>
<a href="https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers" rel="noreferrer" target="_blank">https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers</a> or send a mail to
<a href="mailto:gmx-developers-request@gromacs.org" target="_blank">gmx-developers-request@gromacs.org</a>.</blockquote>
</div>
<br clear="all">
<div><br>
</div>
-- <br>
<div dir="ltr">
<div dir="ltr">
<div>
<div dir="ltr">
<div>
<div dir="ltr">
<div>
<div dir="ltr">
<div>Erik Lindahl &lt;<a href="mailto:erik.lindahl@dbb.su.se" target="_blank">erik.lindahl@dbb.su.se</a>&gt;</div>
<div>Professor of Biophysics, Dept. Biochemistry &amp; Biophysics, Stockholm University</div>
<div>Science for Life Laboratory, Box 1031, 17121 Solna, Sweden</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
-- <br>
Gromacs Developers mailing list<br>
<br>
* Please search the archive at <a href="http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List" target="_blank">
http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List</a> before posting!<br>
<br>
* Can&#39;t post? Read <a href="http://www.gromacs.org/Support/Mailing_Lists" target="_blank">
http://www.gromacs.org/Support/Mailing_Lists</a><br>
<br>
* For (un)subscribe requests visit<br>
<a href="https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers" target="_blank">https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers</a> or send a mail to
<a href="mailto:gmx-developers-request@gromacs.org" target="_blank">gmx-developers-request@gromacs.org</a>.</div>
</blockquote>
</div>
<br>
</div>
</div>
</div>

-- <br>
Gromacs Developers mailing list<br>
<br>
* Please search the archive at <a href="http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List" rel="noreferrer" target="_blank">http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List</a> before posting!<br>
<br>
* Can&#39;t post? Read <a href="http://www.gromacs.org/Support/Mailing_Lists" rel="noreferrer" target="_blank">http://www.gromacs.org/Support/Mailing_Lists</a><br>
<br>
* For (un)subscribe requests visit<br>
<a href="https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers" rel="noreferrer" target="_blank">https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers</a> or send a mail to <a href="mailto:gmx-developers-request@gromacs.org" target="_blank">gmx-developers-request@gromacs.org</a>.</blockquote></div>