opensim-org / opensim-core

SimTK OpenSim C++ libraries and command-line applications, and Java/Python wrapping.
https://opensim.stanford.edu
Apache License 2.0
783 stars 316 forks source link

Cleanup: Moves Millard2012Equilibrium member funcs to anonymous namespace #3617

Closed pepbos closed 10 months ago

pepbos commented 10 months ago

This PR moves some private member functions of Millard2012EquilibriumMuscle to free functions: i.e. a refactor/cleanup.

Currently the outputs of those functions depend partly on the input arguments, making it harder to see what the actual inputs for computing the outputs are. The use of free functions makes this more clear.

Additionally, the use of Vector3 or Vector4 as the output of functions is reduced (by using a designated struct, or splitting into different functions).

Brief summary of changes

Testing I've completed

Unit tests are passing. Verified output of forward integrating RajagopalModel did not change. Performance (wall clock time) of different RajagopalModel simulations did not change.

CHANGELOG.md


This change is Reviewable

tkuchida commented 10 months ago

You might consider naming the functions so that the common parts are at the beginning (e.g., calcFiberForceActive(), calcFiberForcePassiveElastic()). They will then appear next to each other in doxygen and autocomplete lists, and might be easier for users to find. 🐫

pepbos commented 10 months ago

@tkuchida I was under the impression that anonynmous namespace things in cpp files will not end up in doxygen, and I don't see them there. It was also not my intention to show them in the doxygen, since they are helper functions, and not an interface to the muscle. So, not sure I completely follow, could you clarify?

tkuchida commented 10 months ago

@pepbos For methods/variables that are related, it's common to name them starting with the same prefix. That way, they will appear next to each other in alphabetized lists (e.g., autocomplete) and it can be easier to find related things. It's just a convention. Presumably, someone is more likely to be interested in other things related to fiber force (calcFiberForce...()) rather than other things related to the adjectives (e.g., calcPassiveElastic...()) though not necessarily.

pepbos commented 10 months ago

ty @tkuchida for clarifiying, makes sense :)

pepbos commented 10 months ago

@tkuchida , @adamkewley , @nickbianco , ty for reviewing,I implemented your suggestions, let me know what you think