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

Inline calculations #58

Merged
merged 13 commits into from
Apr 25, 2024
Merged

Inline calculations #58

merged 13 commits into from
Apr 25, 2024

Conversation

alyst
Copy link
Contributor

@alyst alyst commented Apr 5, 2024

Switches to in-line criterion_and_gradient!(), as well as in-line gradient_f!() and in-line project_X!().

I have tried to reduce temporary array allocations by using methods like 5-arg mul!() or axpy!() 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 the crieterion_and_gradient!() changes it started to fail, and increasing atol 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 from pub.
The Quartimax criterion based on the original pub 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

@alyst
Copy link
Contributor Author

alyst commented Apr 5, 2024

This is how profiling looks for varimax of 732x30 matrix after this PR:
image

This is zoom into criterion_and_gradient!():
image

Copy link

codecov bot commented Apr 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.68%. Comparing base (9f687b9) to head (70e7980).
Report is 2 commits behind head on main.

❗ Current head 70e7980 differs from pull request most recent head 6c2c0c7. Consider uploading reports for the commit 6c2c0c7 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #58      +/-   ##
==========================================
- Coverage   98.69%   98.68%   -0.01%     
==========================================
  Files          23       23              
  Lines         459      456       -3     
==========================================
- Hits          453      450       -3     
  Misses          6        6              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/methods/methods.jl Outdated Show resolved Hide resolved
@alyst alyst force-pushed the inline_gradient branch 6 times, most recently from 32c00fe to c721436 Compare April 11, 2024 05:58
@alyst
Copy link
Contributor Author

alyst commented Apr 21, 2024

@p-gw Gentle ping :)

@p-gw
Copy link
Owner

p-gw commented Apr 23, 2024

@p-gw Gentle ping :)

I'll most likely get to review it this week.

Copy link
Owner

@p-gw p-gw left a comment

Choose a reason for hiding this comment

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

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.

test/methods.jl Show resolved Hide resolved
test/methods.jl Show resolved Hide resolved
test/methods.jl Show resolved Hide resolved
test/methods.jl Show resolved Hide resolved
test/rotate.jl Outdated Show resolved Hide resolved
Alexey Stukalov added 4 commits April 24, 2024 09:23
@alyst
Copy link
Contributor Author

alyst commented Apr 24, 2024

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.

@p-gw p-gw merged commit b3de7a0 into p-gw:main Apr 25, 2024
3 of 4 checks passed
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.

Switch to in-place criterion_and_gradient!
2 participants