Skip to content

Conversation

@mkhona-nvidia
Copy link
Contributor

Added the third order expansion and step size with line search from XL Li.

Improved test for procrustes_step after discussion with XL Li

@copy-pr-bot
Copy link

copy-pr-bot bot commented Oct 14, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@mkhona-nvidia mkhona-nvidia self-assigned this Oct 14, 2025
@mkhona-nvidia mkhona-nvidia requested a review from skyw October 14, 2025 17:53
@mkhona-nvidia
Copy link
Contributor Author

/ok to test 172c725

@mkhona-nvidia
Copy link
Contributor Author

/ok to test 9859c48

@skyw
Copy link
Contributor

skyw commented Oct 14, 2025

Could you break it to two PRs, one for improving test, one for new functionality?
Review of test only change can be much lighter and faster.

@mkhona-nvidia
Copy link
Contributor Author

/ok to test 6ef6f90

@mkhona-nvidia
Copy link
Contributor Author

mkhona-nvidia commented Oct 14, 2025

Could you break it to two PRs, one for improving test, one for new functionality? Review of test only change can be much lighter and faster.

it's a tiny change, and order=3 and order=2 should work interchangeably. all test are identical for order=2 and order=3 and the new test comparing them only tests convergence speed (less steps needed)

@mkhona-nvidia
Copy link
Contributor Author

/ok to test e4e5d65

Copy link
Contributor

@skyw skyw left a comment

Choose a reason for hiding this comment

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

let me take a stab on the main order 2 and 3 function. If nothing better comes up, we can merge as it is.

A few small things need to be fixed also.

@mkhona-nvidia
Copy link
Contributor Author

/ok to test 5f830fe

@skyw skyw merged commit 9d93954 into NVIDIA-NeMo:main Oct 15, 2025
14 checks passed
@mkhona-nvidia mkhona-nvidia deleted the mkhona/psgd_procrustes_step_dev branch October 15, 2025 02:58
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.

2 participants