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.

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

Feature/sportec tracking #208

Merged
merged 15 commits into from
Jul 30, 2023
Merged

Conversation

koenvo
Copy link
Contributor

@koenvo koenvo commented Jul 22, 2023

  • Deprecated sportec.load and added sportec.load_event
  • Add deserializer for Sportec Tracking data
  • Update docs

…e method. This way the metadata parsing for Sportec can be shared between the event- and trackingdata Deserializers
…ossible and only convert string attributes to float when constructing a Frame. When only_alive or sample_rate are set this will prevent lots of unnecessary type convertions
@koenvo koenvo requested a review from JanVanHaaren July 23, 2023 09:36
Copy link
Contributor

@MKlaasman MKlaasman 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 this PR Koen!

Overall this looks as I expected, I think there is still some value in adding Acceleration to the dataset.

kloppy/_providers/sportec.py Outdated Show resolved Hide resolved
kloppy/_providers/sportec.py Outdated Show resolved Hide resolved

assert player_data.coordinates == Point(x=0.35, y=-25.26)

# We don't load distance right now as it doesn't
Copy link
Contributor

Choose a reason for hiding this comment

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

There is also an Acceleration attribute in the Sportec data, is there a possibility to add this to the dataset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jup. Would like to implement this in a different ticket to first check other providers too.


def _sportec_metadata_from_xml_elm(match_root) -> SportecMetadata:
"""
Load metadata from Sportec XML element. This part is shared between event- and tracking data.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

if player_id != "ball"
},
other_data={},
ball_coordinates=Point3D(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add ball_speed

…vent vs tracking data. Therefore, Provider alone cannot be used to identify a CoordinateSystem. Use DatasetType to determine correct CoordinateSystem. Also refactor construction of DatasetTransformer into DatasetTransformerBuilder, and use it from both TrackingDataDeserializer as EventDataDeserializer.
@UnravelSports
Copy link
Contributor

I've ran it and it seems to be working correctly after you added ball speed and the updated coordinate systems!

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.

The pull request looks good to me! I added two minor suggestions to improve the readability of the code.

dataset_type: DatasetType,
):
from_coordinate_system = build_coordinate_system(
provider, length=length, width=width, dataset_type=dataset_type
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps use the same formatting as for the to_coordinate_system? The similarties and differences between the two calls to build_coordinate_system would become more apparent.

periods = [
Period(
id=1,
start_timestamp=10_000 / SPORTEC_FPS,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is not immediately clear to me where the 10_000, 100_000, 200_000 and 250_000 constant values come from. I suspect it is a Sportec convention. How about moving these constant values to a variable with a more descriptive name? Or adding a quick comment explaining the convention?

@koenvo koenvo merged commit 6b0ec4d into PySport:master Jul 30, 2023
19 checks passed
@koenvo koenvo deleted the feature/sportec-tracking branch July 30, 2023 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants