Closed alyst closed 6 months ago
This is how profiling looks for varimax of 732x30 matrix after this PR:
This is zoom into criterion_and_gradient!()
:
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 98.68%. Comparing base (
9f687b9
) to head (70e7980
). Report is 2 commits behind head on main.:exclamation: Current head 70e7980 differs from pull request most recent head 6c2c0c7. Consider uploading reports for the commit 6c2c0c7 to get more accurate results
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@p-gw Gentle ping :)
@p-gw Gentle ping :)
I'll most likely get to review it this week.
The overall changes look good to me, nice speed up in
rotate
as well. Before we can merge this we need to rebase on the current main branch.
Thank you for the review! I've rebased the PR, the CI tests pass except for nightly, which is due to Ezyme problems not related to the PR.
Switches to in-line
criterion_and_gradient!()
, as well as in-linegradient_f!()
and in-lineproject_X!()
.I have tried to reduce temporary array allocations by using methods like 5-arg
mul!()
oraxpy!()
and made some tweaks in specific rotation methods. But more in-depth code analysis + profiling would be required to tune the performance (in future PRs).Note that I have changed the reference Quartimax loadings (
pub
matrix) in the test: after thecrieterion_and_gradient!()
changes it started to fail, and increasingatol
did not help. So I have checked what is the output of the GPArotation package (from the same authors as the original 2005 publication), and it was also different frompub
. The Quartimax criterion based on the originalpub
loadings is smaller (by ~1e-6), but I suspect that is the result of the rounding error, and the corresponding rotation matrix would have been non-orthogonal. The criterion difference between GPArotation and Quartimax was ~1e-16.Fixes #57