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

Fix PolynomialLR power type. #1440

Merged
merged 2 commits into from
Mar 19, 2025

Conversation

hiyuh
Copy link
Contributor

@hiyuh hiyuh commented Feb 3, 2025

torch.optim.lr_scheduler.PolynomialLR power was typed int, but should be double.
Non-integer power is widely used for common training recipe.
E.g. torchvision's pre-trained semantic segmentation models uses PolynomialLR as main LR scheduler with power = 0.9; https://github.com/pytorch/vision/blob/main/references/segmentation/train.py#L201
See also https://pytorch.org/docs/stable/generated/torch.optim.lr_scheduler.PolynomialLR.html

@hiyuh
Copy link
Contributor Author

hiyuh commented Feb 5, 2025

@dotnet-policy-service agree company="HACARUS"

@hiyuh
Copy link
Contributor Author

hiyuh commented Feb 5, 2025

regarding TorchSharp.TestJIT.TestLoadJIT_3 failed,
i have no idea for now, since this PR would not intend to change any exception handling.

@alinpahontu2912 alinpahontu2912 self-requested a review March 13, 2025 10:34
Copy link
Member

@alinpahontu2912 alinpahontu2912 left a comment

Choose a reason for hiding this comment

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

Hey, LGTM, thanks for the fix :). Can you update your branch and also add the some notes in Releasenotes ?

@alinpahontu2912
Copy link
Member

regarding TorchSharp.TestJIT.TestLoadJIT_3 failed, i have no idea for now, since this PR would not intend to change any exception handling.

It is a flaky test, we will try running again, don't worry

@hiyuh hiyuh force-pushed the fix-PolynomialLR-power-type branch from 802c1ef to 5b76bb7 Compare March 17, 2025 02:27
@hiyuh
Copy link
Contributor Author

hiyuh commented Mar 17, 2025

@alinpahontu2912

  • added RELEASENOTES.md update after rebase.
    • according to CONTRIBUTING.md, rebase is preferred, instead of merge commit.
    • however, it looks you're doing a merge commit in your branch.
    • i'm not sure which is true preference.

@hiyuh hiyuh requested a review from alinpahontu2912 March 17, 2025 02:29
@hiyuh hiyuh force-pushed the fix-PolynomialLR-power-type branch from 5b76bb7 to d1f9b3f Compare March 18, 2025 13:34
@hiyuh hiyuh requested a review from alinpahontu2912 March 18, 2025 13:35
Copy link
Member

@alinpahontu2912 alinpahontu2912 left a comment

Choose a reason for hiding this comment

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

Awesome, thanks 👍

ozanMSFT
ozanMSFT previously approved these changes Mar 19, 2025
Copy link
Contributor

@ozanMSFT ozanMSFT left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks for the contribution.

@alinpahontu2912
Copy link
Member

Hey @hiyuh, one of your PRs got merged, thanks for the contribution! Can you fix the Releasenotes conflict so we can merge this one too ?

Masaru Kimura added 2 commits March 19, 2025 21:06
torch.optim.lr_scheduler.PolynomialLR power was typed int,
but should be double.
Non-integer power is widely used for common training recipe.
E.g. torchvision's pre-trained semantic segmentation models uses
PolynomialLR as main LR scheduler with power = 0.9;
https://github.com/pytorch/vision/blob/main/references/segmentation/train.py#L201
See also https://pytorch.org/docs/stable/generated/torch.optim.lr_scheduler.PolynomialLR.html
@hiyuh hiyuh dismissed stale reviews from ozanMSFT and alinpahontu2912 via 79739db March 19, 2025 12:08
@hiyuh hiyuh force-pushed the fix-PolynomialLR-power-type branch from d1f9b3f to 79739db Compare March 19, 2025 12:08
@hiyuh
Copy link
Contributor Author

hiyuh commented Mar 19, 2025

@alinpahontu2912
done & go ahead. :)

@alinpahontu2912 alinpahontu2912 self-requested a review March 19, 2025 14:07
@alinpahontu2912 alinpahontu2912 merged commit 9a5ac0b into dotnet:main Mar 19, 2025
2 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.

4 participants