Conversation
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (10.76%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #339 +/- ##
===========================================
- Coverage 99.22% 95.09% -4.14%
===========================================
Files 49 66 +17
Lines 3736 5258 +1522
===========================================
+ Hits 3707 5000 +1293
- Misses 29 258 +229 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Alek050
left a comment
Alek050
left a comment
There was a problem hiding this comment.
Hi @ouyang1030,
Thanks for opening this PR. I am really happy with the quality and style of your code. I really appreciate that you kept a very similar format and architecture for writing the parser that is already used in DataBallPy. Before we can merge it we need a few minor and one major point.
- Consider removing inline comments as much as possible. The code should speak for itself. I think the code by itself is clear enough, any necessary additions can be added to the docstrings instead.
- It seems like you build in events that have
NaT. Since the datetime objects are used in synchronisation I would like your thoughts on a fallback date option. For example if a game has now start time, can we just say it starts at a hardcoded time? - This is a big one, but next to the code implementation we need unittests to make sure everything works as expected. This requires testing unique functions to see the outcome is as expected. Let me know when you need help with this.
- Last, you can also update the Changelog, documentation and readme to update the available data providers in this PR.
Other than that I have added some small questions on specific parts on the code because I do not have access to the Fifa data. I would appreciate it if you can answer on question and resolve comments that you fix in the next PR.
Looking forward to finishing this PR soon!
| @@ -0,0 +1,782 @@ | |||
| from ast import In | |||
There was a problem hiding this comment.
Where and for what is this used?
| from databallpy.utils.logging import logging_wrapper | ||
|
|
||
|
|
||
| # FIFA event type mappings to databallpy events |
There was a problem hiding this comment.
Please minimise inline comments, usually they can be left out and fixed with readable code. In this case I think the mappings are perfectly clear without the inline comments. Consider removing.
| "attempt_at_goal": "shot", | ||
| "own_goal": "own_goal", | ||
| "tackle": "tackle", | ||
| "no_event": "tackle", |
There was a problem hiding this comment.
Why would we map a 'no_event' to a 'tackle'?
| away_formation=away_formation, | ||
| country=country, | ||
| ) | ||
| metadata.kickoff_utc = kickoff_time |
There was a problem hiding this comment.
Why do we still need the property kickoff_utc if it is already present in the periods dataframe? I would prefer not to add new properties if it is not necessary.
| home_formation = metadata_json["home_formation"] | ||
| away_formation = metadata_json["away_formation"] |
There was a problem hiding this comment.
are the fifa formations directly in the format DataBallPy expects them to be?
| # Ensure boolean dtype | ||
| event_data["is_successful"] = event_data["is_successful"].astype("boolean") | ||
| event_data.loc[event_data["period_id"] > 5, "period_id"] = -1 | ||
| event_data = event_data.drop(columns=["outcome_additional"]) |
There was a problem hiding this comment.
I see you drop it here. Is there a reason why you did not just do it in the loop? Are there any performance issues?
| "offer_events": offer_events, | ||
| "other_events": other_events, |
There was a problem hiding this comment.
I think you can drop these, although I am not sure what 'offer_events' are. I would prefer to also have 'dribble_events' in here.
| body_part = BODY_PART_MAP.get(event.get("body_type", "other"), "unspecified") | ||
|
|
||
| # Get set piece | ||
| origin = event.get("origin", "") |
There was a problem hiding this comment.
does origin refer to set piece information or to the start of the possession type?
| x_norm = event.get("x_location_start") if "x_location_start" in event else event.get("x") | ||
| y_norm = event.get("y_location_start") if "y_location_start" in event else event.get("y") | ||
|
|
||
| if x_norm is None or pd.isna(x_norm): | ||
| x_norm = 0.5 | ||
| if y_norm is None or pd.isna(y_norm): | ||
| y_norm = 0.5 | ||
|
|
||
| x_norm = max(0.0, min(1.0, float(x_norm))) | ||
| y_norm = max(0.0, min(1.0, float(y_norm))) | ||
|
|
||
| if period_id == 1 and flip_first_half: | ||
| x_norm = 1.0 - x_norm | ||
| y_norm = 1.0 - y_norm | ||
| elif period_id == 2 and flip_second_half: | ||
| x_norm = 1.0 - x_norm | ||
| y_norm = 1.0 - y_norm | ||
|
|
||
| x_start = (x_norm * pitch_dimensions[0]) - (pitch_dimensions[0] / 2.0) | ||
| y_start = (y_norm * pitch_dimensions[1]) - (pitch_dimensions[1] / 2.0) | ||
|
|
||
| if event.get("team_id") == away_team_id: | ||
| x_start *= -1 | ||
| y_start *= -1 |
There was a problem hiding this comment.
This suppart of code seems to be written out multiple times. Consider creating a seperate function for this functionality so you minimise double code.
There was a problem hiding this comment.
Please remove this file. potentially add *.lnk to the .gitignore
2 participants
Support for FIFA event data