Implementation of __eq__ for Events

Open
#1,246 4 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
5/5
Estimated time
Over a week
Newbie friendliness
25/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Stale
Tech stack
python, pytorch
Domain
backend

Research direction

Start at events.CallableEventWithFilter.eq and the engine.on registration path described in the report. Reproduce the two equality comparisons and the EPOCH_COMPLETED handler registrations, then inspect how these events behave as dictionary keys. Done means distinct event/filter combinations retain their identity in containers and the intended API behavior is covered by regression tests.

Written by the indexing model from the issue text.

Description

enhancement needs-discussion

🐛 Bug description

I have a suggestion on how Event objects are implemented. I noticed a strange behavior after upgrading to 0.4.0 and it's related to how __eq__ is implemented for events.CallableEventWithFilter.

I think neither of these should be True:

EPOCH_COMPLETED == EPOCH_COMPLETED(every=2)
EPOCH_COMPLETED(every=2) == EPOCH_COMPLETED(every=3)

because we lose information as soon as we put these in containers. For instance

@engine.on(EPOCH_COMPLETED)
def foo(engine):
    pass

@engine.on(EPOCH_COMPLETED(every=2))
def bar(engine):
    pass

will put <function bar> in engine._event_handlers for <event=EPOCH_COMPLETED, filter=<function CallableEventWithFilter.default_event_filter>> instead of <event=EPOCH_COMPLETED, filter=<function CallableEventWithFilter.every_event_filter>>. The overall behavior of the program is correct, but this resulted into a bug on my side because I was using events as keys in a dictionary.

It looks like Enums are used as both Enums and Enum+filter objects. Sometime they act as Enums and sometimes they act as combined objects. Maybe we should avoid this and use a slightly different API like @engine.on(EPOCH_COMPLETED, every=2)?

I understand this won't be backwards-compatible, but I thought I'd bring this to your attention.

Environment

  • PyTorch Version: 1.6.0
  • Ignite Version: 0.4.1
  • OS: Linux:
  • How you installed Ignite: source
  • Python version: 3.8.3
Dominant language
Python
Stars
4.8k
Forks
726
Avg merge
5d 21h
Merged PRs (30d)
5

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from pytorch/ignite

All issues in pytorch/ignite

Similar issues

More Python issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.