-
-
Notifications
You must be signed in to change notification settings - Fork 617
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
Updated BaseOutputHandler to accept state attributes - Tensorboard #2137
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 @Ishan-Kumar2 !
Looks good. I have a suggestion to remove TrainerStateHandler
and put the new arg to OutputHandler
.
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.
@Ishan-Kumar2 thanks for the update ! I liked the way you handled data type dispatch and I left a comment on how we could improve our loggers.
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 @Ishan-Kumar2
There few issues with docstrings otherwise, looks good.
@@ -179,12 +179,9 @@ class OutputHandler(BaseOutputHandler): | |||
Examples: | |||
|
|||
.. code-block:: python | |||
|
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 think these modifications are wrong, take a look how it is rendered now: https://deploy-preview-2137--pytorch-ignite-preview.netlify.app/generated/ignite.contrib.handlers.tensorboard_logger.html#ignite.contrib.handlers.tensorboard_logger.OutputHandler
Let's also update line 177:
- Helper handler to log engine's output and/or metrics
+ Helper handler to log engine's output, engine's state attributes and/or metrics
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.
This line 182 shouldn't be deleted. That's why the docstring is wrongly rendered.
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 @Ishan-Kumar2
@Ishan-Kumar2 I forgot to ask you to add ignite/ignite/handlers/stores.py Lines 38 to 39 in c6ff5b8
Please send a PR with a fix. By the way, would you like to update other loggers ? |
@Ishan-Kumar2 can you address this comment please for TB and Visdom. Yesterday I also forgot to ask about that. Thanks |
Addresses #2100 (using "fixes" will close the issue)
Description:
Adds feature to log state attributes in Tensorboard
Check list: