Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup: Moves Millard2012Equilibrium member funcs to anonymous namespace #3617

Conversation

pepbos
Copy link
Contributor

@pepbos pepbos commented Nov 20, 2023

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

  • Millard2012EquilibriumMuscle::calcFiberForce->Vec4 split into:
    • CalcFiberForce->double,
    • CalcActiveFiberForce->double,
    • CalcPassiveElasticFiberForce->double` and
    • CalcPassiveDampedFiberForce->double.
  • Millard2012EquilibriumMuscle::calcFv moved to CalcUndampedFiberForceVelocityMultiplier
  • Millard2012EquilibriumMuscle::calcDampedNormFiberVelocity moved to CalcDampedNormFiberVelocity

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

  • no need to update because: refactor of private member functions...

This change is Reviewable

@tkuchida
Copy link
Member

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. 🐫

Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor request related to method naming.

OpenSim/Actuators/Millard2012EquilibriumMuscle.cpp Outdated Show resolved Hide resolved
OpenSim/Actuators/Millard2012EquilibriumMuscle.cpp Outdated Show resolved Hide resolved
OpenSim/Actuators/Millard2012EquilibriumMuscle.cpp Outdated Show resolved Hide resolved
OpenSim/Actuators/Millard2012EquilibriumMuscle.cpp Outdated Show resolved Hide resolved
OpenSim/Actuators/Millard2012EquilibriumMuscle.cpp Outdated Show resolved Hide resolved
OpenSim/Actuators/Millard2012EquilibriumMuscle.cpp Outdated Show resolved Hide resolved
OpenSim/Actuators/Millard2012EquilibriumMuscle.cpp Outdated Show resolved Hide resolved
OpenSim/Actuators/Millard2012EquilibriumMuscle.cpp Outdated Show resolved Hide resolved
mli.fiberPassiveForceLengthMultiplier);
p2Fm =
CalcPassiveFiberDampingForce(fiso, mvi.normFiberVelocity, beta);
fm = CalcFiberForce(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this new implementation re-calculate the same stuff twice? E.g. CalcFiberForce now internally does CalcActiveFiberForce and CalcPassiveFiberElasticForce. Therefore, this refactored version ends up double-computing them to save (I guess?) having to return a single Vec4.

If your intention was to simplify the implicitness of returning a packed Vec4, then I'd recommend that the refactor instead returns a struct with named members (e.g. activeFiberForce, passiveFiberElasticForce, etc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect the compiler to optimize, and remove the repeated calls, I even hinted with inline! Should i verify? Or just not do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved it differently, since it was just fm = aFm + pFm,

I split it into different calls, because somewhere the total was not used, and in most places the parts were not used. So I think currently it has no repeated computations, and is not calculating more than needed.

@pepbos
Copy link
Contributor Author

pepbos commented Nov 21, 2023

@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
Copy link
Member

@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
Copy link
Contributor Author

pepbos commented Nov 21, 2023

ty @tkuchida for clarifiying, makes sense :)

compiler knows best
@pepbos
Copy link
Contributor Author

pepbos commented Nov 21, 2023

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

Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pepbos pepbos merged commit fc79402 into opensim-org:main Nov 23, 2023
6 checks passed
@pepbos pepbos deleted the MoveMillardMemberFuncsToFreeFuncs branch November 23, 2023 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants