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

feat: spatial_metrics_model #146

Open
wants to merge 36 commits into
base: develop
Choose a base branch
from

Conversation

VellaRo
Copy link

@VellaRo VellaRo commented Sep 4, 2024

Added spatialMetricsModel with connvexhull plot (on pitch) and code to run an example.

@VellaRo VellaRo changed the base branch from main to develop September 4, 2024 08:41
@draabe
Copy link
Contributor

draabe commented Sep 6, 2024

Hi @VellaRo,

thanks for the feature addition.

Could you briefly explain how this PR differs from #79 ? That's also in the pipeline and quite ready, including convex hull models.

Also, please make sure to follow style conventions as described in the docs

Thanks!

@VellaRo
Copy link
Author

VellaRo commented Oct 1, 2024

Hi @draabe ,

this PR is a modified version of PR 79 to calculate the convex_hull_area and the effective_playing_space.

…write_response to dump_list_of_dicts , and read_response to load_list_of_dicts ; changed names to be more clear e.g. : data_list_dict to players_sensor_data_dict_list ; added example to main_user_function
…sModel"

This reverts commit 3270fbe, reversing
changes made to 5cabaf0.
…ricsModel for unhandled errors spotted through testing
@VellaRo VellaRo changed the title Feature spatial metrics model feat:spatial_metrics_model Oct 31, 2024
@VellaRo VellaRo changed the title feat:spatial_metrics_model feat: spatial_metrics_model Oct 31, 2024
@manuba95
Copy link
Contributor

manuba95 commented Nov 5, 2024

Hi @VellaRo,
thanks for the contribution. I agree with @draabe that this is quite similar to #79 but since that seems to not have seen progress in quite some time, this may be a good opportunity to get this feature released.
I think generally the code is already well written and main changes depend on a bit more fundamental design choices that will also influence other Models (future and already released).

  1. Flexibility regarding the passed XY-objects. I think this is the most fundamental design choice that will affect how the user can interact with the model. Right know, the user has the option to pass one or two XY-objects to the model to calculate either ConvexHull or EPS. This is similar to the DiscreteVoronoiModel, where the user has to pass two XY-objects (one per team). I think it would be nice if this could be done more flexible, i.e., that the user can pass a single or a list of XY-objects and the calculations get performed for any number. For this PR it would mean that ConvexHull and EPS are basically redundant. The model will calculate the ConvexHull of all XY-objects passed, which would be called EPS if there are two. Of course that would affect the way we approach different methods of the model, e.g., I see that writing the .plot() method that flexible may become interesting. Anyways I think we can use this model to try that approach and maybe decide if we want to apply it to other models, like Voronoi based on the experience. Any ideas from you on that @draabe ?
  2. Handling of "invalid" frames. For basically any model, there is an edge case that leads to improper calculations. E.g., to calculate a convex hull with non-zero area, you need at least three points that are not on one line. The former condition gets currently checked in the .count_valid_pairs() method while the latter gets somewhat checked later. A generalized "invalidity" check is probably quite complicated (maybe except for all-nan frames) as the conditions depend on the respective model, but I think every model should have one designated step that checks if the input values will result in a valid output.
  3. The role of the .fit() method. The .fit() method generally should fit the data to the model but that does not mean that it already produces the respective models results. Here, the .fit() method already calculates the ConvexHull().volume. This leads to the behavior, that the .plot()-method cannot access the already calculated ConvexHulls so it calculates them again. I think a better way to do this is to store the ConvexHull objects in the array and call the .volume or .vertices in the respective .plot() or .area() method.
    I think these are the main issues you should focus on first. You can find some more specific comments below.

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