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

Location coordinate alignment #99

Merged
merged 23 commits into from
Apr 13, 2023

Conversation

rossdrucker
Copy link
Contributor

I noticed while debugging this issue that there was a slight error in the way that the coordinates for shot location data are processed. In the commits in this PR, I've re-aligned the coordinate data to mirror what exists with the ESPN "standard" via the following:

  • Flipped (x, y) coordinates to create a horizontal court, as is the standard (e.g. this shot chart)
  • Aligned coordinates so that (0, 0) corresponds to center court
  • Home team corresponds to right side of court per ESPN convention

After making these changes in the get_shot_loc() function, I updated shot chart plotting functions to utilize this new coordinate alignment. In these functions, I changed the base plot to use the sportyR package for three reasons:

  1. Removes dependency on static court coordinates data set. sportyR makes updating court features (e.g. three-point line distance) easier to maintain than the static coordinates currently shipped with the package. Not needing these should hopefully reduce some of the load time of the package as well
  2. Has the potential to allow a user to supply their own theme for shot charts if desired. If this is something that should be introduced to ncaahoopR, I can add in arguments to the appropriate plotting functions that can then be passed to sportyR::geom_basketball() to color the court as desired by the user.
  3. This aligns with the adjusted coordinates pulled from ESPN

Other minor edits I made are really just to use consistent namespacing throughout the package and reduce notes when running devtools::check(); can help more with this as necessary and desired.

Let me know if anything here can/should be updated. I greatly appreciate the review and consideration!

@lbenz730
Copy link
Owner

Hi Ross! This is extremely helpful!

I am going to let the package stay as is for the remainder of this season but will review all this and integrate your changes following the end of the Final Four in two weeks time.

Thanks so much for this outstanding contribution.

@lbenz730
Copy link
Owner

@rossdrucker I will review this week and merge

@rossdrucker
Copy link
Contributor Author

@lbenz730 Just calling your attention to the commits I just pushed tonight for your review. They're mostly intended to help clear some (not all) of the NOTEs and WARNINGs when running devtools::check() should you be interested in putting this on CRAN. I know that's a bit outside the scope of the initial PR, so happy to revert those commits if you'd like, but figured they might be helpful to have anyways. Happy to work on resolving more of these NOTEs and WARNINGs as well if desired/needed

@lbenz730 lbenz730 merged commit 01a22a0 into lbenz730:master Apr 13, 2023
@lbenz730
Copy link
Owner

@rossdrucker This looks fantastic. Thank you for your tremendous contribution. Feel free to make additional contributions as you see fit. Your support and collaboration is much appreciated

@lbenz730
Copy link
Owner

@rossdrucker I made one small tweak to address a bug in game_shot_chart(). Otherwise looks fantastic. I don't want to make more work for you put if you're looking for more stuff to do:

  • Would love a redesign of the background of the shot charts to look a bit cleaner. I would defer to your design decisions
  • I think you've mostly addressed Clean Up Shot Locations #40 but wondering if the court is now 1 foot back (correct for D-1 Men compared to about 2-3 years ago), and maybe we also can fix the FT issue if that's still problematic in the ESPN. The issue which I don't know how to fix is that ESPN seems to plot points much further beyond the 3 point arc than what the "truth should be"
  • Heat maps like Improve shot chart graphics #48

Don't feel any pressure to do any of this but those are some related tasks if you're up for it. Happy to offer co-authorship for your assistance. In any case, you've already done outstanding outstanding work and made the package better!

NOTE to self: closed #74 after this PR

@rossdrucker rossdrucker deleted the location-coordinate-alignment branch April 14, 2023 07:08
@rossdrucker
Copy link
Contributor Author

@lbenz730 Happy to take some of that on as I have time to do so, although it may be a bit more spread out over the offseason. May be worth revisiting some of those issues and continuing discussion there to keep things organized. Just as a quick note, the three-point arc is moved back to the correct spot. It's customizable though through sportyR, so if we want to build that in as a parameter, I'm confident we can find a way to do so. I'll take a look at the FT issue though; that should be relatively straightforward

@lbenz730
Copy link
Owner

Thanks @rossdrucker ! No time pressure at all. Really appreciate your help w/ this. I merged #100 and pushed a update to DESCRIPTION to make you listed as an author. You've done some really helpful work already and I look forward to continuing our collaboration!

@rossdrucker
Copy link
Contributor Author

Much appreciated @lbenz730, I'm looking forward to continuing collaborating as well! I'll probably open up a few issues, but quickly wanted to check if the goal is to put this on CRAN?

@lbenz730
Copy link
Owner

I'm not entirely sure what that would entail. Given the frequent updates that may be annoying to maintain and keep re-submitting to CRAN but if you think it's not too much work we can consider it.

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.

None yet

2 participants