Skip to content

Conversation

@koenvo
Copy link
Contributor

@koenvo koenvo commented Sep 29, 2025

No description provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

This CommonDataFormatCoordinateSystem should instead follow this approach.

Please include it inside this file, instead of the separate file you created. That way everything is organised in 1 place.

class SecondSpectrumCoordinateSystem(ProviderCoordinateSystem):
    """
    Second Spectrum coordinate system.

    Uses a pitch with the origin at the center and the y-axis oriented from
    bottom to top. The coordinates are in meters.
    """

    @property
    def provider(self) -> Provider:
        return Provider.SECONDSPECTRUM

    @property
    def origin(self) -> Origin:
        return Origin.CENTER

    @property
    def vertical_orientation(self) -> VerticalOrientation:
        return VerticalOrientation.BOTTOM_TO_TOP

    @property
    def pitch_dimensions(self) -> PitchDimensions:
        if self._pitch_length is not None and self._pitch_width is not None:
            return MetricPitchDimensions(
                x_dim=Dimension(
                    -1 * self._pitch_length / 2, self._pitch_length / 2
                ),
                y_dim=Dimension(
                    -1 * self._pitch_width / 2, self._pitch_width / 2
                ),
                pitch_length=self._pitch_length,
                pitch_width=self._pitch_width,
                standardized=False,
            )
        else:
            return MetricPitchDimensions(
                x_dim=Dimension(None, None),
                y_dim=Dimension(None, None),
                pitch_length=None,
                pitch_width=None,
                standardized=False,
            )

Choose a reason for hiding this comment

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

done with this the class is well implemented now.

…ion retrieving and get block on handling the class into kloppy.domain...
@UnravelSports
Copy link
Contributor

@stephTchembeu can you:

  • Make sure the test_cdf.py does not contain any reference to your local files? Instead, you can use existing pff files in kloppy/tests/files/
  • I see you have a TODO Open question: should the serializer make sure the data is in the right format, and do a transformation if not in the right format? yes normally.. The answer is yes, so always transform to correct coordinate system and attacking static home away, although I think youre already doing this. If this is old, just remove the TODO.
  • Clean up the inline comments in serialize.py.
  • Remove prints
  • Make sure all the docstrings follow the format are in Google style docstrings
  • Import VERSION from cdf to run tests automatically on latest version (e.g cdf/files/v{VERSION}/schema etc.)
  • Run "black" on the files you've created to align code formatting.

@koenvo what would be the best way to structure the functionality. Like, what did you have in mind for something like below?

from kloppy import common_data_format

common_data_format.store() # opposite of .load?

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.

3 participants