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

give an option to store EpochOutputStore data on engine.state #1974

Merged
merged 4 commits into from May 1, 2021
Merged

give an option to store EpochOutputStore data on engine.state #1974

merged 4 commits into from May 1, 2021

Conversation

radekosmulski
Copy link
Contributor

Implements #1939

Description: When EpochOutputStore is attached and a name is provided, at EPOCH_COMPLETED engine.state.{name} is set to eos.data (in other words, eos.data is stored on engine.state).

Check list:

  • New tests are added (if a new feature is added)
  • New doc strings: description and/or example code are in RST format
  • Documentation is updated (if required)

@github-actions github-actions bot added the module: contrib Contrib module label Apr 27, 2021
@radekosmulski radekosmulski changed the title Eos store data give an option to store EpochOutputStore data on engine.state Apr 27, 2021
@sdesrozis
Copy link
Contributor

@radekosmulski Thank you ! Please check my suggestion.

Comment on lines 53 to 54
"""Attaching `reset` method at EPOCH_STARTED and
`update` method at ITERATION_COMPLETED."""
Copy link
Collaborator

@vfdev-5 vfdev-5 Apr 27, 2021

Choose a reason for hiding this comment

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

@radekosmulski please also update the docstring here.
Let's also put .. versionchanged:: as it is explained here : https://github.com/pytorch/ignite/blob/master/CONTRIBUTING.md#writing-documentation
We can put 0.5.0 version.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Apr 27, 2021

@radekosmulski thanks for the PR, looks good, just a docstring update remains. If you would like to move this class into core in another PR, it is OK for me (maybe even prefered way to do that).

@radekosmulski
Copy link
Contributor Author

I added the changes in two commits - one updates the docstrings, the other the example. I am not sure if the update to the example is needed?

Generally, not sure if this is the right way to do it 🙂

Will move it out of contrib in a separate PR 🙂

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.

LGTM! Thanks @radekosmulski

@vfdev-5 vfdev-5 merged commit e8418eb into pytorch:master May 1, 2021
@vfdev-5 vfdev-5 added this to In progress in 0.4.5 via automation May 28, 2021
@vfdev-5 vfdev-5 moved this from In progress to Done in 0.4.5 May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: contrib Contrib module
Projects
No open projects
0.4.5
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants