Add under pressure qualifier#296
Conversation
|
@probberechts Do you have any feedback on this pull request? Thanks. |
|
If an event (e.g., a clearance) is executed under pressure and the ball goes out subsequently, the Apart from that, this PR looks good to me. |
|
Thanks for the feedback @probberechts! Adjusted the code accordingly. |
|
@koenvo, this should be ready to merge! |
# Conflicts: # kloppy/infra/serializers/event/statsbomb/specification.py # kloppy/tests/test_statsbomb.py
|
I've started to question whether a |
I agree, but I think there are two dimensions to consider. The first dimension is whether the action happens under pressure or not. The second dimension is the pressure intensity, given that the action happens under pressure. For instance, StatsBomb event data only provides information on the first dimension, although information on the second dimension could be derived from StatsBomb 360 data, if available. |
|
Thanks for the input @JanVanHaaren. I agree that there are two dimensions. Whether there is pressure or not should go into a
To be precise, it could also be implemented like this: class PressureLevel(Enum):
LOW = "low"
MEDIUM = "medium"
HIGH = "high"
@dataclass
class UnderPressureQualifier(Qualifier):
value: bool | PressureLevel
def to_dict(self):
if isinstance(self.value, bool):
return {f"is_{self.name}": self.value}
elif isinstance(self.value, PressureLevel):
return {f"{self.name}": self.value.value}
else:
raise TypeError("Value must be either a boolean or a PressureLevel enum.")
@dataclass
class PressingIntensity(ScalarStatistic):
"""Pressing intensity"""One downside of this approach is that the qualifier value would vary depending on the data provider. Ultimately, I'm not sure what the best approach is, and I'm fine with the current implementation as well. I just wanted to point out this alternative to encourage further discussion. Let me know your thoughts! |
|
My preference would go to have a distinct For StatsBomb we would thus only set the Wdyt? class PressureLevel(Enum):
LOW = "low"
MEDIUM = "medium"
HIGH = "high"
@dataclass
class Statistic(ABC):
name: str = field(init=False)
@dataclass
class OrdinalStatistic(Statistic):
levels: Type[Enum] # Enum class defining the levels
value: Enum # Current level, an instance of the Enum
def compare(self, other: "OrdinalStatistic") -> int:
"""
Compare this statistic with another OrdinalStatistic.
Returns:
-1 if this is less than other
0 if they are equal
1 if this is greater than other
"""
if self.levels != other.levels:
raise ValueError("Cannot compare statistics with different levels")
return (
list(self.levels).index(self.value) - list(self.levels).index(other.value)
)
@dataclass
class PressingIntensity(OrdinalStatistic):
"""Pressing Intensity"""
def __post_init__(self):
self.name = "Pressing Intensity" |
|
What do you think about my proposal @probberechts? If you agree, I can work on the implementation. |
|
Goh, I don't know. Since others do not seem to have an opinion about this, I guess you can decide. 😃 |
|
@probberechts okay than I would keep the Once we start adding support with kloppy the level of pressure, then the 'PressingIntensity' could be used for it as described above in my dummy code block. Code should be ready to merge! |
I think you previously fixed this, but it seems it's now adding the |
|
No, you are right, I reintroduced the issue during resolving of the merge conflicts earlier on. But I fixed it again, should be ready to merge now! |
I want to add
UnderPressureQualifieras a possible qualifier for our events and create support for parsing this with StatsBomb.