add before and after events filter

[louis-she]

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)

[vfdev-5]

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

[louis-she]

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]

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:

[sadra-barikbin]

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

[sadra-barikbin]

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]

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]

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]

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]

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

[louis-she]

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

[vfdev-5]

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

[vfdev-5]

Thanks @louis-she , looks good to me !