Add DuelEvent #204
Add DuelEvent #204
Conversation
1. Remained with Pieter’s initial proposal of only adding: AERIAL, GROUND, LOOSE_BALL & SLIDING_TACKLE 2. StatsBomb: Checked qualifiers with “name” instead of id, since ids are not consistent in StatsBomb open data. As per StatsBomb helpdesk. 3. Added a method: .get_qualifier_values() . Which returns a list of Qualifiers instead of .get_qualifier_value(), that returns the first Qualifier. 4. Also Added NEUTRAL as outcome, since this is provided in wyscout_v2
kloppy/domain/models/event.py
Outdated
| if qualifiers: | ||
| return qualifiers | ||
|
|
||
| return None |
There was a problem hiding this comment.
I would expect this function to always return List type.
So always return qualifiers
| result = DuelResult.LOST | ||
|
|
||
| return dict( | ||
| result=result, |
There was a problem hiding this comment.
This can be simplified to result=DuelResult.WON if outcome else DuelResult.LOST
| else: | ||
| result = DuelResult.LOST | ||
|
|
||
| return {"result": result, "qualifiers": qualifiers} |
There was a problem hiding this comment.
I would do similar as above using DuelResult.WON if outcome_name in DUEL_WON_NAMES else DuelResult.LOST
| **duel_event_kwargs, | ||
| **generic_event_kwargs, | ||
| ) | ||
| new_events.append(duel_event) |
There was a problem hiding this comment.
Should this happen before the shot/clearance/miscontrol/pass event or after (using .append)?
| @@ -386,9 +418,9 @@ def deserialize(self, inputs: WyscoutInputs) -> EventDataset: | |||
| **recovery_event_args, **generic_event_args | |||
| ) | |||
| elif raw_event["eventId"] == wyscout_events.DUEL.EVENT: | |||
| takeon_event_args = _parse_takeon(raw_event) | |||
|
Thanks for this pull request! Some minor comments and can you make sure to merge master in as there seem to be some conflicts. |
# Conflicts: # kloppy/domain/models/event.py # kloppy/domain/services/event_factory.py # kloppy/infra/serializers/event/opta/deserializer.py # kloppy/infra/serializers/event/statsbomb/deserializer.py # kloppy/infra/serializers/event/wyscout/deserializer_v2.py # kloppy/infra/serializers/event/wyscout/deserializer_v3.py # kloppy/tests/files/wyscout_events_v3.json # kloppy/tests/test_wyscout.py
|
@koenvo should be good to merge. |
requirements.txt
Outdated
| pytz~=2023.3 | ||
| python-dateutil~=2.8.2 | ||
| setuptools~=67.8.0 |
There was a problem hiding this comment.
What is the intention behind having a setuptools entry in requirements.txt? I think you can’t install Setuptools from the requirements.txt file anyway because you need to have Setuptools installed in order to install anything (with Pip) in the first place.
There was a problem hiding this comment.
@MKlaasman can you update this one?
requirements.txt
Outdated
| pre-commit | ||
| kloppy~=3.11.0 |
There was a problem hiding this comment.
Kloppy shouldn't be a requirement of Kloppy.
There was a problem hiding this comment.
@MKlaasman can you update this one?
There was a problem hiding this comment.
Not sure how I introduced these...
Deleted all requirements not present in current master, namely:
kloppy~=3.11.0
pytz~=2023.3
python-dateutil~=2.8.2
setuptools~=67.8.0
Please double check whether this is correct.
# Conflicts: # kloppy/infra/serializers/event/opta/deserializer.py # kloppy/tests/test_opta.py # kloppy/tests/test_wyscout.py
PR related to issue described in #135
Added DuelEvent for:
• StatsBomb, Opta & WyScout (v2 & v3); including tests
• Added DuelTypes:
AERIAL,GROUND,LOOSE_BALL&SLIDING_TACKLE• Added .get_qualifier_values() to get a list of qualifiers instead of the first value.
@probberechts can you take a look, whether this suffices?