Skip to content

improved test for procrustes step and added third order expansion#58

Merged
skyw merged 5 commits into
NVIDIA-NeMo:mainfrom
mkhona-nvidia:mkhona/psgd_procrustes_step_dev
Oct 15, 2025
Merged

improved test for procrustes step and added third order expansion#58
skyw merged 5 commits into
NVIDIA-NeMo:mainfrom
mkhona-nvidia:mkhona/psgd_procrustes_step_dev

Conversation

@mkhona-nvidia

@mkhona-nvidia mkhona-nvidia commented Oct 14, 2025

Copy link
Copy Markdown
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-pr-bot Bot commented Oct 14, 2025

Copy link
Copy Markdown

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

mkhona-nvidia commented Oct 14, 2025

Copy link
Copy Markdown
Contributor Author

/ok to test 172c725

@mkhona-nvidia
replace inplace with torch.add
9859c48 
Signed-off-by: mikail <mkhona@nvidia.com>
@mkhona-nvidia

mkhona-nvidia commented Oct 14, 2025

Copy link
Copy Markdown
Contributor Author

/ok to test 9859c48

@skyw

skyw commented Oct 14, 2025

Copy link
Copy Markdown
Contributor

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
fixed error in 3rd order procrustes
6ef6f90 
Signed-off-by: mikail <mkhona@nvidia.com>
@mkhona-nvidia

mkhona-nvidia commented Oct 14, 2025

Copy link
Copy Markdown
Contributor Author

/ok to test 6ef6f90

@mkhona-nvidia

mkhona-nvidia commented Oct 14, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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
3rd order convergence is only about number of steps, changed test to …
e4e5d65 
…reflect that

Signed-off-by: mikail <mkhona@nvidia.com>
@mkhona-nvidia

mkhona-nvidia commented Oct 14, 2025

Copy link
Copy Markdown
Contributor Author

/ok to test e4e5d65

skyw
skyw requested changes Oct 15, 2025
View reviewed changes

@skyw skyw left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread emerging_optimizers/psgd/procrustes_step.py
Comment thread tests/test_procrustes_step.py Outdated
Comment thread tests/test_procrustes_step.py Outdated
Comment thread tests/test_procrustes_step.py Outdated
@mkhona-nvidia
addressed PR comments and made step counting change in test
5f830fe 
Signed-off-by: mikail <mkhona@nvidia.com>
@mkhona-nvidia

mkhona-nvidia commented Oct 15, 2025

Copy link
Copy Markdown
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

Reviewers

@skyw skyw skyw approved these changes

Assignees

@mkhona-nvidia mkhona-nvidia

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mkhona-nvidia @skyw