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

Filters don't work on custom events #1555

Closed
amatsukawa opened this issue Jan 13, 2021 · 7 comments · Fixed by #1587
Closed

Filters don't work on custom events #1555

amatsukawa opened this issue Jan 13, 2021 · 7 comments · Fixed by #1587

Comments

@amatsukawa
Copy link
Contributor

CallableEventWithFilter does not work with custom events:

from ignite.engine import Engine, EventEnum
    
def train_step(engine, batch):
    engine.fire_event(TestEvents.TEST_EVENT)
engine = Engine(train_step)

class TestEvents(EventEnum):
    TEST_EVENT = 'test_event'
engine.register_events(*TestEvents)

@engine.on(TestEvents.TEST_EVENT(every=100))
def foo(engine):
    print("hello world")

def forever():
    while True:
        yield 0.
    
engine.run(forever(), max_epochs=10, epoch_length=1)

The error is:

RuntimeError: Unknown event name '<event=TEST_EVENT, filter=<function CallableEventWithFilter.every_event_filter.<locals>.wrapper at 0x7fb7d1244710>>'
@amatsukawa
Copy link
Contributor Author

The issue appears to be some dependence on this map here:

event_to_attr = {

I have not dug through the code to see what the dependence actually is, but it seems I should be able to do every=100 on custom events that are not necessarily bound to iteration or epoch lengths.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Jan 13, 2021

Hey @amatsukawa, nice to see you here again :) thanks for reporting, I'll take a look !

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Jan 13, 2021

Well, it is a usage issue and bad documentation. Here is how it should be

from ignite.engine import Engine, EventEnum
    

class TestEvents(EventEnum):

    TEST_EVENT = 'test_event'

    
def train_step(engine, batch):
    engine.fire_event(TestEvents.TEST_EVENT)
    engine.state.test_event += 1
    
    
engine = Engine(train_step)
engine.register_events(*TestEvents, event_to_attr={TestEvents.TEST_EVENT: "test_event"})


@engine.on(TestEvents.TEST_EVENT(every=3))
def foo(engine):
    print("hello world", engine.state.test_event)


def forever():
    while True:
        yield 0.
    
engine.run(forever(), max_epochs=10, epoch_length=1)

As we want to filter out events, we have to have event counters and thus define a map between event name and state attribute which should be also incremented...
I'd like to refactor a bit Engine class such that counters are automatically incremented, but this is still WIP.

@amatsukawa
Copy link
Contributor Author

Ah, awesome! Thanks for the clarification!

@amatsukawa
Copy link
Contributor Author

amatsukawa commented Jan 14, 2021

Actually @vfdev-5 just want to confirm one thing: is it necessary to increment engine.state.test_event as you have done, and what are the rules on the value string in the enum, and the value of event_to_attr (does it refer to the property of engine.state, the value string of the enum, or both?)

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Jan 14, 2021

Yes, currently it is necessary to increment state's attribute as we check when to skip handlers trigerring according to engine.state.test_event. The str value in enum class : TestEvents.TEST_EVENT == 'test_event' is event's unique id and can be anything. Just in my example its value is accidentally the same as for state's attribute. But for example for Events.EPOCH_COMPLETED == "epoch_completed", its state attribute is state.epoch. Same for the custom event. We registered the mapping between event and state's attribute by

engine.register_events(*TestEvents, event_to_attr={TestEvents.TEST_EVENT: "test_event"})

and this engine.state.test_event = 0 was initialized.

@jrieke
Copy link
Contributor

jrieke commented Jan 28, 2021

I added some docs changes in #1587. @vfdev-5 Please have a look if that works out, then I can re-create the docs and finish the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants