-
-
Notifications
You must be signed in to change notification settings - Fork 641
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
Edit LRFinder to have more than one parameter #2704
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @Jacob208M
I left some comments on how to improve the code.
We also need to write some tests. Please check https://github.com/pytorch/ignite/blob/master/tests/ignite/handlers/test_lr_finder.py
For development tips please see: https://github.com/pytorch/ignite/blob/master/CONTRIBUTING.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update @Jacob208M
We need to work more on the code to make it better. I left few comments on what to do. Feel free to ask questions if it is not clear.
Hi @Jacob208M , thanks for this PR. Would you like any help? |
@sadra-barikbin Can you review this changes is everything fine please? |
@Jacob208M I reviewed your changes and put some comments. Are they clear? |
Yes, I'll manage it |
@sadra-barikbin I just updated my code as you showed me. I haven't added ParamGroupScheduler because I wasn't sure about it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for delay. I left some comments and change requests.
@sadra-barikbin I did changes but now I got my tests failed with info
Is it a problem with param_scheduler? What should I do with it? |
Thanks. Could you please push the changes to see where the error arises from? |
Done |
@sadra-barikbin Hi, sorry for my absence, I was busy because my studies have started. I committed your changes but I still see there is an error, now it looks like that: Can you check it please? |
|
I pulled new changes and now there is no error from get_param, but simple assertion False. I'm not sure why. Can you look at it? |
"Check code formatting" step in jobs is failing. Let's fix it first. To install ./tests/run_code_style.sh install You can see errors locally with: ./tests/run_code_style.sh lint |
@Jacob208M can you please follow and apply suggestions from this comment: #2704 (comment). so we can advance on this feature. Thanks! |
I think is done, flake8 doesn't throw any errors, tests are passing. Could you look at this, please? |
@Jacob208M can you also run code formatting script:
|
def test_multi_opt(lr_finder, dummy_engine_mulitple_param_groups, to_save_mulitple_param_groups, dataloader, step_mode): | ||
start_lr = [0.1, 0.1, 0.01] | ||
end_lr = [1.0, 1.0, 1.0] | ||
dummy_engine = dummy_engine_mulitple_param_groups |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to tell you calling ./tests/run_code_style.sh fmt
applies formatting rules automatically and there were no need to redefine variables with short names. Applying that command would result in:
def test_multiple_optimizers(
lr_finder, dummy_engine_mulitple_param_groups, to_save_mulitple_param_groups, dataloader, step_mode
):
start_lr = [0.1, 0.1, 0.01]
end_lr = [1.0, 1.0, 1.0]
with lr_finder.attach(
dummy_engine_mulitple_param_groups,
to_save_mulitple_param_groups,
start_lr=start_lr,
end_lr=end_lr,
step_mode=step_mode,
) as trainer:
trainer.run(dataloader)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way for more information on Python style guides you could read PEP 8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jacob208M @sadra-barikbin Thank both of you guys for this PR !
Looks good to me
Fixes #2703
Description:
Editing FastaiLRFinder to have more than one parameter