Skip to content

Conversation

gruebel
Copy link
Contributor

@gruebel gruebel commented Dec 2, 2020

Fixes #1406

Description:
That was brutal. Especially the param_scheduler.py is such a mess 😄 A lot of the time I had to guess and debug to find out what types are used/returned.
Since the private class torch.optim.lr_scheduler._LRScheduler has no types defined, I had to add type: ignore quite often 😢

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Dec 3, 2020

@gruebel thanks a lot for the PR ! Maybe, it would be more simple to split the PR into multiple ones (simplier reviewing) and either we can try to rework param_scheduler.py before passing mypy inside ?

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

@gruebel I reviewed all files except ignite/contrib/handlers/param_scheduler.py , so let's split this PR into 2: 1 = everything without ignite/contrib/handlers/param_scheduler.py and 2 = only ignite/contrib/handlers/param_scheduler.py

I left a comment to improve a bit tqdm handler. Otherwise, looks good to me.

@gruebel
Copy link
Contributor Author

gruebel commented Dec 3, 2020

Sure, no problem. I removed the changes on param_scheduler.py and will create a new PR after this one is done 😄

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

Thanks @gruebel !

@vfdev-5 vfdev-5 merged commit 825be07 into pytorch:master Dec 5, 2020
@gruebel gruebel deleted the activate-mypy-for-contrib-handlers branch December 7, 2020 08:34
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.

MyPy: improve ignite.contrib module
2 participants