Closed DriesDeprest closed 3 days ago
@koenvo @JanVanHaaren did you guys find the time to take a look this PR? I'd like to continue the effort, but first want to check whether you agree with the implementation proposal.
Thank you for your proposal, @DriesDeprest! Your approach generally looks good to me.
The following thoughts crossed my mind while looking through the code.
mm:ss
format instead of the HH:mm:ss
format?What are your thoughts, @probberechts?
Thanks for reviewing @JanVanHaaren!
1.
I noticed the same similarities and raised it here: https://github.com/PySport/kloppy/issues/301. How I see it, you have two options on how to get the actual formation / position of a team / player for each event. a) You create a specific event (FormationChange/PositionChange) for it and using the state builder, you can get the state formation/position at time of execution of the event. b) You have a formation/position_change attribute in the Team/Player object, instead of a formation/position attribute, and with the right method position/formation() you can get the actual formation / position of a team / player for each event.
For formations, we have historically chosen to go for implementation a) but for positions we are now building implementation b).
The reason why I prefer option b) over option a) is because it also offers you a standardized data format for storing formation changes of a team and position changes of players of a football game across vendors, besides the ability to get the actual formation / position of a team / player for each event
See https://github.com/PySport/kloppy/issues/277#issuecomment-2015474935 for why we don't store the end timestamp of a position change.
I believe so, but will double-check once I continue the implementation, after we agree on the high level implementation.
Closing this PR, since we added position changes with: https://github.com/PySport/kloppy/pull/326.
Created this draft PR related to: https://github.com/PySport/kloppy/issues/277, to get initial feedback on the implementation prior to starting updating all the deserializers. This is also based on the discussion made here: https://github.com/probberechts/kloppy/pull/1.
As a sake of example, I've started with updating the Statsomb deserializer to support the Player.positions.
@JanVanHaaren @koenvo happy to hear what you guys think.