Fix transform orientation

[probberechts]

This PR makes the following changes:

Breaking changes

• The previous behaviour of Orientation.FIXED_HOME_AWAY, Orientation.FIXED_AWAY_HOME, Orientation.HOME_TEAM, and Orientation.AWAY_TEAM was counter-intuitive. For example, when the orientation is FIXED_HOME_AWAY the user expects that the home team will play from left-to-right the entire dataset, while it changes during halftime. Therefore, the definition of these orientations was changed as follows:

Orientation	Definition	Corresponding old orientation
HOME_AWAY	The home team plays from left to right in the first period. The away team plays from left to right in the second period.	FIXED_HOME_AWAY
AWAY_HOME	The away team plays from left to right in the first period. The home team plays from left to right in the second period.	FIXED_AWAY_HOME
STATIC_HOME_AWAY	The home team plays from left to right in both periods.	HOME_TEAM
STATIC_AWAY_HOME	The away team plays from left to right in both periods.	AWAY_TEAM

• AttackingDirection.HOME_AWAY was renamed to AttackingDirection.LTR (indicating that the home team attacks left-to-right) and AttackingDirection.AWAY_HOME was renamed to AttackingDirection.RTL (indicating that the home team attacks right-to-left) to avoid confusion with the identically named orientations.
• The attacking_direction attribute was removed from Period. Instead, the attacking direction is now defined for each DataRecord as a property derived from the dataset's orientation.

Fixes

• Fix the orientation transform logic (Fixes Bug in orientation transform #175).
• Add support for orientation transforms on games with extra time.
• Guarantee that the dataset's pitch dimensions and coordinate system metadata attributes are consistent.
• Adapt orientation attribute of Datafactory deserializer from Orientation.HOME_TEAM to Orientation.HOME_AWAY. The home team always starts left-to-right and changes at half-time.
• Adapt orientation attribute of Skillcorner deserializer from Orientation.HOME_TEAM/Orientation.AWAY_TEAM to Orientation.HOME_AWAY/Orientation.AWAY_HOME. The teams change halves at half-time.

Features

• Raise a warning when the orientation of a dataset cannot be determined (e.g., when the first period is not recorded for a tracking dataset)

[koenvo]

Looks good to me. The checks with the Period.id makes it way more clear than the (non-working) attacking_direction concept. Did you check for any attacking_direction leftovers?

[probberechts]

Thanks. I did a grep-search on "attacking_direction" and I think the attribute is not used anywhere else anymore.

[koenvo]

I think the failing part of the test should be removed as attacking_direction is redundant.

[probberechts]

That sounds like a band-aid solution 😉 Being redundant does not mean that it shouldn't be correct and consistent. I also don't know what is incorrect in the SecondSpectrum deserializer: the orientation attribute or the attacking direction attributes. I didn't really look into it. I only made the observation that one of them must be wrong.

But a relevant question is what we want to do with the attacking_direction. Should we remove it entirely from the code base / deprecate it? Or keep it as an alternative interface for the orientation of a dataset?

[koenvo]

That sounds like a band-aid solution 😉 Being redundant does not mean that it shouldn't be correct and consistent. I also don't know what is incorrect in the SecondSpectrum deserializer: the orientation attribute or the attacking direction attributes. I didn't really look into it. I only made the observation that one of them must be wrong.

But a relevant question is what we want to do with the attacking_direction. Should we remove it entirely from the code base / deprecate it? Or keep it as an alternative interface for the orientation of a dataset?

Ah sorry, didn't want to propose a "just remove the failing test" to solve the issue :-) My idea is/was to remove the attacking_direction entirely as it keeps confusing me. On the other hand, it is useful to know to which direction a team is attacking but I'm not sure yet what should happen when you change the orientation. I guess it should be transformed too.

Currently the attacking_direction is inferred from the first frame in a period. Maybe it's better to just provide the helper which determines the attacking_direction and remove it entirely from the objects itself.

[probberechts]

My take on this is that the orientation is defined by the attacking direction in each half. Hence, if you transform the orientation, the attacking directions should be updated too. That is how I implemented it in this PR.

If a dataset has a known (FIXED_)HOME/AWAY_TEAM orientation, it implies that you also know in which direction a team is attacking. Hence, the attacking direction attribute is redundant, but it could be a convenient alternative interface to infer the attacking direction when working with individual frames since each frame has a period and therefore also an attacking direction attribute.

If a dataset has a BALL_OWNING_TEAM or ACTION_EXECUTING_TEAM orientation, the attacking direction attribute is currently useless and confusing. If you follow my reasoning that the attacking direction and orientation are linked, the attacking direction should not be defined for a period but for each team in each frame or possession. Therefore, I set the attacking direction of periods to "NOT_SET" for these orientations.

Anyway, I am also in favor of removing the attacking direction entirely from the codebase since I was confused by it for a very long time too and that would simplify the code.

Instead of inferring the attacking direction from the first frame in a period, I would modify the helper to infer the orientation from the first frame in both periods.

[koenvo]

@probberechts and I had a call to discuss this PR. The summary (Pieter can you correct where needed?):

Current findings:

1. The AttackingDirection is super confusing and seasoned developers don't understand how it works
2. The current behaviour of Orientation.FIXED_HOME_AWAY and Orientation.FIXED_AWAY_HOME are counter-intuitive: the user expects when orientation is FIXED_HOME_AWAY the home team will play from left-to-right the entire dataset. Right now this is not how it works.

To got forward the following changes are required:

1. Remove the AttackingDirection entirely. It can be derived from Orientation combined with Period in case it's needed. Can be added as a property on Period
2. Change the behaviour of FIXED_HOME_AWAY. When this orientation is used the home team will be playing from left-to-right the entire dataset.
3. Replace HOME_TEAM with HOME_AWAY, and AWAY_TEAM with AWAY_HOME to indicate the orientation of the first-period only, and that it will switch per period. This is the typical orientation for raw tracking data.

@UnravelSports @JanVanHaaren can you have a look at the above?

[UnravelSports]

The way you put it now is the way I expected the behaviour to be. So yes, that all sounds good to me.

As an aside and fwiw, I've used this PR in beta for about a week and have not had any issues with it.

[probberechts]

I've updated the first message in this thread with an overview of all changes.

The only thing that I implemented differently from what we discussed is the AttackingDirection. I decided to keep it, as it was useful to easily determine the attacking direction of a specific frame, but I moved it from Period to DataRecord such that it works for every Orientation.

[probberechts]

I've fixed merge conflicts and changed the "FIXED_" prefix to "STATIC_".

[koenvo]

@probberechts could you fix merge conflict? Would like to merge this PR.

[probberechts]

@koenvo Done!

[koenvo]

Thanks!