Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improvements to Opta and Wyscout deserializers #198

Merged
merged 15 commits into from
Jul 13, 2023
Merged

Improvements to Opta and Wyscout deserializers #198

merged 15 commits into from
Jul 13, 2023

Conversation

DriesDeprest
Copy link
Contributor

@DriesDeprest DriesDeprest commented Jun 19, 2023 •
edited
Loading

Made the following changes / fixes:
Opta:

  • Mark events as counter attacks
  • Fix pass events without end coordinates
  • Introduce shot results

Wyscout

  • Handle infractions better
  • Start timestamp from 0 again at start of period 2.

@JanVanHaaren JanVanHaaren changed the title Feature/opta fixes Improvements to Opta and Wyscout deserializers Jun 20, 2023
JanVanHaaren
JanVanHaaren reviewed Jun 20, 2023
View reviewed changes
Copy link
Collaborator

@JanVanHaaren JanVanHaaren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull request! I've added a few questions to help me better understand the proposed changes.

kloppy/infra/serializers/event/opta/deserializer.py Show resolved Hide resolved
kloppy/infra/serializers/event/wyscout/deserializer_v3.py Show resolved Hide resolved
kloppy/infra/serializers/event/wyscout/deserializer_v3.py Outdated Show resolved Hide resolved
kloppy/tests/test_wyscout.py Show resolved Hide resolved
kloppy/infra/serializers/event/wyscout/deserializer_v3.py Show resolved Hide resolved
@DriesDeprest
Copy link
Contributor Author

I don't understand how my latest commit could cause the failing checks?
https://github.com/PySport/kloppy/actions/runs/5321135197/jobs/9635847489?pr=198
@JanVanHaaren do you have an idea what is going on?

@JanVanHaaren
Copy link
Collaborator

@DriesDeprest I believe that the issue is caused by a regression bug, see actions/setup-python#682. Yesterday's build used Python 3.7.16, whereas today's build used Python 3.7.17.

@DriesDeprest
Copy link
Contributor Author

@DriesDeprest I believe that the issue is caused by a regression bug, see actions/setup-python#682. Yesterday's build used Python 3.7.16, whereas today's build used Python 3.7.17.

What do you propose we do with this? Wait until they fix it or pin the Python version 3.7.16 for now in our build?

@JanVanHaaren
Copy link
Collaborator

What do you propose we do with this? Wait until they fix it or pin the Python version 3.7.16 for now in our build?

@DriesDeprest I think we can fix the Python version to 3.7.16 for the time being (see neptune-ai/neptune-client@f6bca6e for example), unless @koenvo prefers a different approach.

koenvo added a commit that referenced this pull request Jun 21, 2023
@koenvo
Update test.yml
1607958 
Temporary pin python 3.7 to 3.7.16 due to actions/setup-python#682.

See also #198
@koenvo
Copy link
Contributor

koenvo commented Jun 21, 2023

I'm trying to pin it at #199

@koenvo
Copy link
Contributor

koenvo commented Jun 21, 2023

Can you merge master in please?

@DriesDeprest
Merge branch 'master' into feature/opta-fixes
8ebb250 
# Conflicts:
#	.github/workflows/test.yml
@DriesDeprest
Copy link
Contributor Author

@JanVanHaaren checks are now passing (thx Koen!).
Are your comments resolved with my answers and new commits? :)

@DriesDeprest
Fix: Make it so that we don't have FOUL_COMMITTED events for events d…
979f939 
…escribing a player was fouled.

Opta description of foul event:
"Indicates a foul has been committed. The event comes in pairs, with one for the team committing the foul (has outcome = 0) and another for the team fouled (outcome = 1)."
@DriesDeprest
black format
b7e8887
@JanVanHaaren
Copy link
Collaborator

@DriesDeprest Thank you for your changes. Two of my questions are still open. It would be good to hear your thoughts.

@DriesDeprest
Copy link
Contributor Author

@JanVanHaaren I've tagged you in the different remaining open questions / conversations with follow-up comments or questions, feel free to mark them as resolved when clear for you or pose additional questions.

@JanVanHaaren
Copy link
Collaborator

@DriesDeprest Unfortunately, both @koenvo and I can't see your comments and questions.

@DriesDeprest
Copy link
Contributor Author

@JanVanHaaren I made the same mistake as described here: https://github.com/orgs/community/discussions/10369
But should have submitted my review now, and you should now see my comments / questions 😄

JanVanHaaren
JanVanHaaren approved these changes Jun 29, 2023
View reviewed changes
Copy link
Collaborator

@JanVanHaaren JanVanHaaren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries! The pull request looks good to me. Thank you.

@koenvo
Copy link
Contributor

koenvo commented Jul 6, 2023

It seems there are some conflicts that prevent merging this PR. Can you merge master in?

@DriesDeprest
Merge branch 'master' into feature/opta-fixes
8f6954a 
# Conflicts:
#	kloppy/infra/serializers/event/opta/deserializer.py
#	kloppy/tests/files/wyscout_events_v3.json
#	kloppy/tests/test_wyscout.py
@DriesDeprest
Copy link
Contributor Author

Done!

@koenvo koenvo merged commit e4c892b into PySport:master Jul 13, 2023
19 checks passed
@DriesDeprest DriesDeprest deleted the feature/opta-fixes branch August 11, 2023 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JanVanHaaren JanVanHaaren JanVanHaaren approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DriesDeprest @JanVanHaaren @koenvo @akuu666