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

Fixing metrica epts loader #159

Merged
merged 7 commits into from
Oct 14, 2022

Conversation

jcnunezmoreno
Copy link
Contributor

Copy link
Contributor

@koenvo koenvo 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!

Could you update a Metrica EPTS test so it will test this behavior? And one minor code improvement comment.

@@ -54,11 +54,11 @@ def _frame_from_row(

other_data = {}
for sensor in other_sensors:
player_sensor_field_str = f"player_{player.player_id}_{sensor.channels[0].channel_id}"
player_sensor_val = None if player_sensor_field_str not in row else row[player_sensor_field_str]
Copy link
Contributor

Choose a reason for hiding this comment

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

player_sensor_val = row.get(player_sensor_field_str)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@jcnunezmoreno
Copy link
Contributor Author

Thanks for this PR!

Could you update a Metrica EPTS test so it will test this behavior? And one minor code improvement comment.

Thanks you so much for the quick response. I've added a test now, and fixed the if statement. :)

@koenvo
Copy link
Contributor

koenvo commented Oct 11, 2022

Sorry for the delay. Thanks for the changes! Looking good to me. Can you also run code formatting?

@jcnunezmoreno
Copy link
Contributor Author

Sorry for the delay. Thanks for the changes! Looking good to me. Can you also run code formatting?

Done! Thanks!!

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.

2 participants