Incoming PR - Improvement to ExponentialSpringForce

[fcanderson]

Incoming PR - Improvement to ExponentialSpringForce

[fcanderson]

Thanks for the response Ayman. You expressed exactly the same preference as @sherm1.

~Sixteen days ago I submitted a format-only PR, and @sherm1 quickly turned that PR around. The format changes are now merged.

A few days later, I submitted the PR with the substantive changes. Tests passed, but waiting on a review. As soon as the substantive changes are merged, I'll submit the OpenSim PR for my ExponentialSpringForce wrapper.

[sherm1]

As a reviewer, I would prefer to see a separate PR first that corrects only formatting. With that promise, I don't have to look at every line, just do a basic sanity check. Once that's merged, the substantive PR can go in with only substance and no whitespace fluff so it can get a serious review.

[fcanderson]

Ok. Very good. I will proceed as you describe. I'll do a PR that corrects ONLY formatting. Then, I'll do the PR with the substantive changes. Thanks, Sherm.

[aymanhab]

Hi Clay, Typically format changes PRs are separate as they're easy to review. I'd do that first, then material changes to simbody can be reviewed separately next. Ayman

…

On Sun, Sep 25, 2022, 7:26 AM Clay Anderson ***@***.***> wrote: @sherm1 <https://github.com/sherm1> @aymanhab <https://github.com/aymanhab> I hope you both are doing well. While wrapping Simbody::ExponentialSpringForce for use in OpenSim, I thought of and implemented a better way to handle the transition between μₛ and μₖ. The new implementation is entirely in Simbody; it's all under the covers as far as OpenSim is concerned. The new implementation allowed 1 state and a fair amount of code to be removed from class ExponentialSpringForce and related classes. It runs about 10% faster and, I believe, is also more accurate. I've done a fair amount of testing. There may be a minor complication for the PR though. I discovered that the last merge into the Simbody master contained some formatting issues in the file Simbody/src/ExponentialSpringForce.cpp. For example, an entire section of the file is indented, making some line lengths longer than 80 chars, and some method calls have awkward line breaks. The changes may be due to some auto formatting done by Visual Studio. The formatting may not actually be problematic (nightly tests are passing I imagine). There is a rationale for indenting 800+ lines of code; it's just not space efficient and comment lines go past 80 chars. I've made all the changes needed for the new implementation (code and comments). I have not however changed the formatting, thinking it would make it difficult for a reviewer to spot the substantive changes that I made. Correcting the formatting will result in like 800 more lines of code being added to the list of changes, as GitHub sees it. It seems to me there are at least 3 reasonable options for how to handle the PR. I'd like to pick the one that is easiest on the reviewer. Which would be the best option? 1. Correct all the formatting before I submit the PR. That is, the implementation changes and format corrections would all be reviewed together in 1 pass. 2. Submit the PR without the formatting corrections. And then after the substantive changes have been reviewed and accepted, and if the formatting should be modified, I would then submit another round of changes to correct the formatting before the merge into the master. 3. Create a separate branch with the sole aim of correcting the formatting issues and get that taken care of before I submit the PR for the new implementation. As always, thanks for the time you spend on this stuff! Best, Clay — Reply to this email directly, view it on GitHub <#744>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA6JY4BWUCUCBENF53PMY33WABOKBANCNFSM6AAAAAAQVDS3CE> . You are receiving this because you were mentioned.Message ID: ***@***.***>