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

Added how-to guide: Loggers #58

Merged
merged 6 commits into from
Nov 6, 2021

Conversation

anirudhb11
Copy link
Contributor

Resolves #44,
A how-to guide for loggers in Ignite. Demonstrates the usage with one logger (ClearML) and how the usage can be extended to other loggers

@vfdev-5 vfdev-5 requested a review from Priyansi October 25, 2021 15:28
@Priyansi
Copy link
Collaborator

Thank you for this amazing PR @anirudhb11 . Just a few structural changes and we should be good to merge.

Also, I was thinking if we could move the comments inside the code in the notebook as a text cell above that and add the comments as steps there (1, 2, 3, ...). It will render better on the website.

@anirudhb11
Copy link
Contributor Author

anirudhb11 commented Oct 27, 2021

Thank you for this amazing PR @anirudhb11 . Just a few structural changes and we should be good to merge.

Also, I was thinking if we could move the comments inside the code in the notebook as a text cell above that and add the comments as steps there (1, 2, 3, ...). It will render better on the website.

Thanks @Priyansi, have made some changes to address comments raised by you

@Priyansi
Copy link
Collaborator

Hey @anirudhb11 , the code looks good now however my concern was not to remove the comments altogether rather add them in the text cell above so that we have a little more explanation rather than just code. For example - Where to initialize the ClearMLLogger, what are the other handlers like WeightScaler, WeightHist, etc that we can attach and why. You don't need to explain the part before and after (setting up trainer, checkpoint, etc).
Hope this is clear. Feel free to ask any more questions!

@anirudhb11
Copy link
Contributor Author

Hey @anirudhb11 , the code looks good now however my concern was not to remove the comments altogether rather add them in the text cell above so that we have a little more explanation rather than just code. For example - Where to initialize the ClearMLLogger, what are the other handlers like WeightScaler, WeightHist, etc that we can attach and why. You don't need to explain the part before and after (setting up trainer, checkpoint, etc). Hope this is clear. Feel free to ask any more questions!

Thanks for the clarification, will add a description on these lines.

@vfdev-5
Copy link
Member

vfdev-5 commented Nov 5, 2021

@anirudhb11 could you please update the PR and we could land it. Thanks a lot !

@anirudhb11
Copy link
Contributor Author

@anirudhb11 could you please update the PR and we could land it. Thanks a lot !

@vfdev-5 thanks for reminder, apologies for the delayed response, have added a brief explanation regarding the usage as requested by @Priyansi

@vfdev-5
Copy link
Member

vfdev-5 commented Nov 5, 2021

@anirudhb11 Thanks for the update! I let @Priyansi review this PR and merge if everything is ok.

By the way, we are using <!--more--> tag (see https://github.com/pytorch-ignite/examples/blob/main/generate.py#L34-L56) to show short description of the guide in the list. For example https://raw.githubusercontent.com/pytorch-ignite/examples/main/how-to-guides/06-data-iterator.ipynb

Could you please add that as well.

@Priyansi
Copy link
Collaborator

Priyansi commented Nov 5, 2021

Thanks so much @anirudhb11 for adding the explanation! We should be good to merge once @vfdev-5 's comment is resolved.

@anirudhb11
Copy link
Contributor Author

Have added a <!--more--> tag to state the dataset and model being used in the task

Copy link
Member

@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 a lot the PR @anirudhb11
LGTM!

@vfdev-5 vfdev-5 merged commit d38f078 into pytorch-ignite:main Nov 6, 2021
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.

Logging in Ignite
3 participants