-
-
Notifications
You must be signed in to change notification settings - Fork 610
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
add before and after events filter #2727
add before and after events filter #2727
Conversation
@louis-she thansk for the PR ! |
I think it's ready to be reviewed. One thing to be discussed is that should we add a @trainer.on(Events.EPOCH_COMPLETED(after=10) & Events.EPOCH_COMPLETED(before=30))
def foo1():
# called on epoch > 10 and < 30
pass Currently with |
ignite/engine/events.py
Outdated
if not ( | ||
(event_filter is not None) | ||
^ (every is not None) | ||
^ (once is not None) | ||
^ (before is not None) | ||
^ (after is not None) | ||
): |
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.
Hi @louis-she , thanks for the PR. I think it is valid to have both before
and after
specified. What's your thought? @vfdev-5 , What's your thought?
By the way if all components of XOR become True, if statement incorrectly does not raise an error. We could do instead:
if not ( | |
(event_filter is not None) | |
^ (every is not None) | |
^ (once is not None) | |
^ (before is not None) | |
^ (after is not None) | |
): | |
if sum(( | |
event_filter is not None, | |
every is not None, | |
once is not None, | |
before is not None, | |
after is not None) | |
) != 1: |
ignite/engine/events.py
Outdated
before: a value specifying the step that event should be fired before | ||
after: a value specifying the step that event should be fired after |
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 step
is suitable only for ITERATION_*
events and could be misleading here.
before: a value specifying the step that event should be fired before | |
after: a value specifying the step that event should be fired after | |
before: a value specifying the number of occurrence that event should be fired before | |
after: a value specifying the number of occurrence that event should be fired after |
@@ -76,6 +89,12 @@ def __call__( | |||
if (once is not None) and not (isinstance(once, numbers.Integral) and once > 0): | |||
raise ValueError("Argument once should be integer and positive") | |||
|
|||
if (before is not None) and not (isinstance(before, numbers.Integral) and before >= 0): |
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 If statement and the one in line 95 are not covered by tests. You can cover them in test_custom_events.py::test_callable_events_with_wrong_inputs
.
We could accept both |
For the |
I think introducing @trainer.on(Events.EPOCH_COMPLETED(after=10, before=30))
def foo1():
# called on epoch > 10 and < 30
pass @vfdev-5 , What's your thought? |
Yes, I agree. We can just allow after and before args together and thus do not need to introduce complicated & operator. |
d25de02
to
6cf12f7
Compare
comments fixed and rebased, ready to review again @sadra-barikbin |
@louis-she could you please fix mypy issues: https://github.com/pytorch/ignite/actions/runs/3180145927/jobs/5183390359#step:12:12 |
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 @louis-she , looks good to me !
Description:
add before and after events filter
Check list: