Edit LRFinder to have more than one parameter#2704
Conversation
vfdev-5
left a comment
vfdev-5
left a comment
There was a problem hiding this comment.
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
vfdev-5
left a comment
vfdev-5
left a comment
There was a problem hiding this comment.
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. |
sadra-barikbin
left a comment
sadra-barikbin
left a comment
There was a problem hiding this comment.
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 installYou 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.
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.
By the way for more information on Python style guides you could read PEP 8.
vfdev-5
left a comment
vfdev-5
left a comment
There was a problem hiding this comment.
@Jacob208M @sadra-barikbin Thank both of you guys for this PR !
Looks good to me
3 participants
Fixes #2703
Description:
Editing FastaiLRFinder to have more than one parameter