Skip to content

add before and after events filter#2727

Merged
vfdev-5 merged 9 commits intopytorch:masterfrom
louis-she:add-start-after-event-filter
Oct 7, 2022
Merged

add before and after events filter#2727
vfdev-5 merged 9 commits intopytorch:masterfrom
louis-she:add-start-after-event-filter

Conversation

@louis-she
Copy link
Copy Markdown
Contributor

@louis-she louis-she commented Sep 30, 2022 •
edited
Loading

Description:

add before and after events filter

@trainer.on(Events.EPOCH_COMPLETED(before=10))
def foo1():
    pass

@trainer.on(Events.EPOCH_COMPLETED(after=10))
def foo2():
    pass

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: engine Engine module label Sep 30, 2022
@louis-she louis-she changed the title add before and after events filter [WIP] add before and after events filter Sep 30, 2022
@vfdev-5
Copy link
Copy Markdown
Collaborator

vfdev-5 commented Sep 30, 2022

@louis-she thansk for the PR !
@sadra-barikbin could you please review

@louis-she louis-she changed the title [WIP] add before and after events filter add before and after events filter Oct 1, 2022
@louis-she
Copy link
Copy Markdown
Contributor Author

louis-she commented Oct 1, 2022 •
edited
Loading

I think it's ready to be reviewed. One thing to be discussed is that should we add a __and__ operator to the Event? Like this:

@trainer.on(Events.EPOCH_COMPLETED(after=10) & Events.EPOCH_COMPLETED(before=30))
def foo1():
    # called on epoch > 10 and < 30
    pass

Currently with | or __or__, I think we can not achieve this with event filter only

sadra-barikbin
sadra-barikbin suggested changes Oct 1, 2022
View reviewed changes
ignite/engine/events.py Outdated
Comment on lines +74 to +84
if not (
(event_filter is not None)
^ (every is not None)
^ (once is not None)
^ (before is not None)
^ (after is not None)
):
Copy link
Copy Markdown
Collaborator

@sadra-barikbin sadra-barikbin Sep 30, 2022

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:

Suggested change
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
Comment on lines +67 to +68
before: a value specifying the step that event should be fired before
after: a value specifying the step that event should be fired after
Copy link
Copy Markdown
Collaborator

@sadra-barikbin sadra-barikbin Oct 1, 2022

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.

Suggested change
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

ignite/engine/events.py
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):
Copy link
Copy Markdown
Collaborator

@sadra-barikbin sadra-barikbin Oct 1, 2022

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.

@sadra-barikbin
Copy link
Copy Markdown
Collaborator

sadra-barikbin commented Oct 1, 2022

I think it's ready to be reviewed. One thing to be discussed is that should we add a __and__ operator to the Event? Like this:

@trainer.on(Events.EPOCH_COMPLETED(after=10) & Events.EPOCH_COMPLETED(before=30))
def foo1():
    # called on epoch > 10 and < 30
    pass

Currently with | or __or__, I think we can not achieve this with event filter only

We could accept both before and after in the CallableEventWithFilter.__call__ and use self.before_and_after_event_filter(before: Optional[int], after: Optional[int]) internally.

@louis-she
Copy link
Copy Markdown
Contributor Author

louis-she commented Oct 3, 2022 •
edited
Loading

We could accept both before and after in the CallableEventWithFilter.call and use self.before_and_after_event_filter(before: Optional[int], after: Optional[int]) internally.

For the called on epoch > 10 and < 30 scenario, it'll work. Another possible scenario is called every x epochs that after y epochs. What do you think?

@sadra-barikbin
Copy link
Copy Markdown
Collaborator

sadra-barikbin commented Oct 3, 2022

We could accept both before and after in the CallableEventWithFilter.call and use self.before_and_after_event_filter(before: Optional[int], after: Optional[int]) internally.

For the called on epoch > 10 and < 30 scenario, it'll work. Another possible scenario is called every x epochs that after y epochs. What do you think?

I think introducing & operator should be discussed in terms of its usefulness, but just in the case of mixing before and after we can do it as follows:

@trainer.on(Events.EPOCH_COMPLETED(after=10, before=30))
def foo1():
    # called on epoch > 10 and < 30
    pass

@vfdev-5 , What's your thought?

@vfdev-5
Copy link
Copy Markdown
Collaborator

vfdev-5 commented Oct 3, 2022

Yes, I agree. We can just allow after and before args together and thus do not need to introduce complicated & operator.

@louis-she louis-she force-pushed the add-start-after-event-filter branch from d25de02 to 6cf12f7 Compare October 4, 2022 06:53
@louis-she
Copy link
Copy Markdown
Contributor Author

louis-she commented Oct 4, 2022

comments fixed and rebased, ready to review again @sadra-barikbin

@vfdev-5
Copy link
Copy Markdown
Collaborator

vfdev-5 commented Oct 4, 2022

@louis-she could you please fix mypy issues: https://github.com/pytorch/ignite/actions/runs/3180145927/jobs/5183390359#step:12:12

@vfdev-5 vfdev-5 requested a review from sadra-barikbin October 4, 2022 08:13
vfdev-5
vfdev-5 approved these changes Oct 4, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Thanks @louis-she , looks good to me !

sadra-barikbin
sadra-barikbin approved these changes Oct 4, 2022
View reviewed changes
@vfdev-5 vfdev-5 merged commit f51f7c0 into pytorch:master Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vfdev-5 vfdev-5 vfdev-5 approved these changes

+1 more reviewer

@sadra-barikbin sadra-barikbin sadra-barikbin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

module: engine Engine module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@louis-she @vfdev-5 @sadra-barikbin