lbenz730 / ncaahoopR

An R package for working with NCAA Basketball Play-by-Play Data
MIT License
198 stars 48 forks source link

Location coordinate alignment #99

Closed rossdrucker closed 1 year ago

rossdrucker commented 1 year ago

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:

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 commented 1 year ago

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 commented 1 year ago

@rossdrucker I will review this week and merge

rossdrucker commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

@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:

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.