remove matrix computation for calc_spiral_wrap_point #3463
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a minor refactoring of the cylinder wrapping points computation.
The
WrapCylinder::_make_spiral_path()
function computes a rotation matrix, transposes it, multiplies is, and proceeds to use a single column. This rotation matrix can essentially be omitted.The
WrapCylinder::_make_spiral_path()
function is often called during a simulation using e.g. the Rajagopal model, and takes up quite some time. This was also mentioned in #3164 , but that PR was not merged. This PR essentially takes one element of #3164 , i.e eliminating the computation of a rotation matrix.Performance Comparison
Small change, and small perf boost of 3% for Rajagopal.
Brief summary of changes
The changes are to
WrapCylinder::_make_spiral_path()
:m = ...
on line 660 is removed, saving a matrix multiplication and transpose.CalcSpiralWrapPoint
is redefined as a lambda function within_make_spiral_path
theta
is computed using the arctan rather than the acos, to capture the sign.Testing I've completed
Running the rajagopal model, I verified that the computed wrapping points are the same up to error of
~1e-14
.Looking for feedback on...
I would be interested to hear if you think it is worth revisiting #3164 , or perhaps chopping it up into smaller PRs such as these.
CHANGELOG.md
This change is