-
-
Notifications
You must be signed in to change notification settings - Fork 654
Improve docs for custom events #1587
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
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 @jrieke !
I left few comments to improve things
docs/source/concepts.rst
Outdated
@engine.on(Events.STARTED) | ||
def fire_custom_events(engine): | ||
engine.fire_event(CustomEvents.CUSTOM_STARTED) | ||
engine.state.custom_started += 1 # increase state counter for this event (required) |
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.
Let's say this is required for filtering only and can be omitted if no filtering is done. And another note to add is that this is a temporary solution and subject to change in future releases.
docs/source/concepts.rst
Outdated
engine.register_events(*CustomEvents) | ||
engine.register_events( | ||
*CustomEvents, | ||
event_to_attr={ |
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 part is not required actually. I'd put everything in the following way:
- simple custom events without filtering (current doc)
- custom events with filtering:
- engine.state should have corresponding attributes
- event_to_attr is required when
register_events
- should increase counters (subject to be removed)
@vfdev-5 Ok I restructured it as you suggested. I removed all changes from the original docs and added the 3 steps for filtering as a paragraph below it. |
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.
A minor fix to add, otherwise it is OK. Thanks @jrieke !
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.
Few more comments
Btw, you can see the rendering at https://deploy-preview-1587--pytorch-ignite-preview.netlify.app/concepts.html
Co-authored-by: vfdev <vfdev.5@gmail.com>
Co-authored-by: vfdev <vfdev.5@gmail.com>
Co-authored-by: vfdev <vfdev.5@gmail.com>
Co-authored-by: vfdev <vfdev.5@gmail.com>
Co-authored-by: vfdev <vfdev.5@gmail.com>
Co-authored-by: vfdev <vfdev.5@gmail.com>
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.
LGTM, thanks @jrieke !
Fixes #1555
Description: Improved the docs on how to create custom events according to the example in #1555.
Check list: