p-gw / FactorRotations.jl

Rotation methods for factor analysis and principal component analysis in Julia
https://p-gw.github.io/FactorRotations.jl/
MIT License
7 stars 2 forks source link

Fix Varimax #54

Closed alyst closed 6 months ago

alyst commented 7 months ago

As I noticed in #48, criterion(::Varimax) does not center the columns (but criterion_and_gradient() does), which means that it does not calculate the variances. I've fixed that, and added the test that criterion() gives the same result as criterion_and_gradient(), when available.

But if in the long run you consider switching to a unified in-place criterion_and_gradient!(grad::Union{AbstractVector, Nothing}, ...), which would skip gradient calculation if grad === nothing, this PR would be irrelevant. Right now the profiling shows that for a 62x24 matrix rotation ~10% is spent in the array allocation (both gradient calculation and projection).

p-gw commented 6 months ago

But if in the long run you consider switching to a unified in-place _criterion_andgradient!(grad::Union{AbstractVector, Nothing}, ...), which would skip gradient calculation if grad === nothing, this PR would be irrelevant. Right now the profiling shows that for a 62x24 matrix rotation ~10% is spent in the array allocation (both gradient calculation and projection).

This looks like an interesting change, but would require the user to implement the in-place version instead (or additionally) to the already existing criterion_and_gradient. If the performance gain is large enough I think a switch might be worth it. I've added a new issue for this (#57).

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.69%. Comparing base (c2bb45f) to head (8459fc6).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #54 +/- ## ========================================== + Coverage 91.68% 98.69% +7.00% ========================================== Files 23 23 Lines 457 459 +2 ========================================== + Hits 419 453 +34 + Misses 38 6 -32 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.