-
Notifications
You must be signed in to change notification settings - Fork 67
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
Substitutions, players temporarily leaving pitch in Kloppy EventData model and dataframe #362
Comments
There are two general types of events that are related to players going off the pitch and returning on the pitch.
Hence, I would argue that we need at least two different event types. Substitutions class SubstitutionEvent(Event):
player_in: Player | None
player_out: Player However, this does not work well with kloppy's current event data model. The The alternative is to split it into two event types: class SubstitutionInEvent(Event):
"""A player comes on the pitch after a substitution."""
class SubstitutionOutEvent(Event):
"""A player goes off the pitch after a substitution.""" Additionally, this has the advantage that it automatically works well with the dataframe representation. In the object-oriented format, I don't really care whether a substitution is a single object or two objects, but I think that in the dataframe representation, it should definitely be split into two rows. Adding the replacement player in the Temporarily leaving the pitch I think here the main issue is the naming of event types. I agree that people could easily mistakenly assume that these event types are related to substitutions, especially when you are familiar with Opta data. As an alternative to "Player on", I like "Player returns". However, I do not have any ideas for a better name for "Player Off". Maybe it is not really needed to change these names and we can solve it with proper documentation. For reference, this is how kloppy and the main data providers currently do it: kloppy
StatsBomb
Opta
Wyscout |
Thanks for clearly laying it out @probberechts. But why would it be a problem to keep the substitution event as @dataclass(repr=False)
@docstring_inherit_attributes(Event)
class SubstitutionEvent(Event):
"""
SubstitutionEvent
Attributes:
event_type (EventType): `EventType.SUBSTITUTION` (See [`EventType`][kloppy.domain.models.event.EventType])
event_name (str): `"substitution"`,
replacement_player (Player): See [`Player`][kloppy.domain.models.common.Player]
"""
replacement_player: Player
position: Optional[PositionType] = None
event_type: EventType = EventType.SUBSTITUTION
event_name: str = "substitution" where the |
This comment was marked as resolved.
This comment was marked as resolved.
I wouldn't say it is a problem per se. I just think it would be more elegant if we split it up for various reasons. First, it automatically would work well with the dataframe representation. Second, the Third, the This is how I would approach it if I were starting from scratch. Whether it's worth making these changes is a different question. Personally, I don't use the substitution events, so it's not something I’m particularly invested in. 🤷
Thanks, I've edited my previous comment. |
Okay, thanks for explaining. I'm also in doubt whether it's worth making these changes. Maybe a balance between not having to refactor too much and not introducing too much breaking changes but still supporting a valid dataframe representation, can be found by adding a "replacement_player" column in the dataframe representation? I personally only need the |
This has now been fixed by #361 right? |
No. This is a discussion thread about
|
Before 3.16.0, in the Opta Parser specifically,
PLAYER_ON
andPLAYER_OFF
are used to describe a substitution with these two individual events.In (currently officially not yet released) version 3.16.0 we add a breaking change to this by adding the
SUBSTITUTION
event. Where theSUBSTITUTION
event is now the combination ofPLAYER_ON
andPLAYER_OFF
events in case of a substitution. However, thePLAYER_ON
andPLAYER_OFF
events can also relate to players temporarily retiring from the pitch as shared by @DriesDeprest in #361.Some ideas for behavior as discussed by @DriesDeprest @probberechts and me for the Kloppy EventData model should be:
PLAYER_ON
andPLAYER_OFF
more specific to meanPLAYER_RETIRES
andPLAYER_RETURNS
for any event that is unrelated to a substitution.SUBSTITUTION
event and assign thereplacement_player
the Id of the player coming on. (This is implemented in a commit in #333PLAYER_ON
andPLAYER_OFF
explicitly as they are confusing and ambiguous. Similarly to how we did away withHOME_AWAY
orientation in favor ofSTATIC_HOME_AWAY
and they were explicitly renamed.Idea for the implementation in the dataframe representation:
replacement_player
in thereceiving_player
field for theSUBSTITUTION
event.GENERIC:player off
andGENERIC:player on
in favor ofGENERIC:player retires
andGENERIC: player returns
events respectively.Let me know if I forgot anything or if anyone has any suggestions on this!
The text was updated successfully, but these errors were encountered: