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: Update "OOA Behind Pitcher" function #299

Merged
merged 2 commits into from
Dec 13, 2022
Merged

Conversation

zach-hopkins
Copy link
Contributor

Proposed feature which simply adds an argument to the "pitcher_fielding" statcast pull in order to allow for "view" changes (options are: Fielder, Fielding Team, Batting, Batting Team, Pitcher). Helpful for pulling OOA and RAA for defense behind a particular pitcher and retrieving team fielding information. Set default to previous hardcode "Fielder"

@schorrm
Copy link
Collaborator

schorrm commented Dec 12, 2022

Looks good.

  1. Move the new argument to be last for now? @tjburch do we care that much about breaking compat here
  2. Add to tests?
  3. Is there anything that needs updates for the docs?

@@ -8,12 +8,12 @@
from .utils import norm_positions, sanitize_statcast_columns

@cache.df_cache()
def statcast_outs_above_average(year: int, pos: Union[int, str], min_att: Union[int, str] = "q") -> pd.DataFrame:
def statcast_outs_above_average(year: int, pos: Union[int, str], view: str = "Fielder", min_att: Union[int, str] = "q") -> pd.DataFrame:
pos = norm_positions(pos)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should have a docstring here in the first place, but it would be nice to include the options for view in that docstring. If you could start with the following and add the options, that would be awesome.

	"""Scrapes outs above average from baseball savant for a given year and position

	Args:
		year (int): Season to pull
		pos (Union[int, str]): Numerical position (e.g. 3 for 1B, 4 for 2B). Catchers not supported
		min_att (Union[int, str], optional): Integer number of attempts required or "q" for qualified. 
			Defaults to "q".

	Raises:
		ValueError: Failure if catcher is passed

	Returns:
		pd.DataFrame: Dataframe of defensive OAA for the given year and position for players who have met
			the given threshold
	"""

@tjburch
Copy link
Collaborator

tjburch commented Dec 12, 2022

Hey, thanks Zach, this is a clever addition to make this more useful.

Agree with @schorrm on moving the argument to the end. I added a review about a docstring so we know what all can be put there.

For the test, make sure this one still runs with the default value, then add a second just like it for a different option of view.

For the docs, just add a note in this file with the alternate options for views.

Thanks again!

@zach-hopkins
Copy link
Contributor Author

zach-hopkins commented Dec 13, 2022

Hey, thanks Zach, this is a clever addition to make this more useful.

Agree with @schorrm on moving the argument to the end. I added a review about a docstring so we know what all can be put there.

For the test, make sure this one still runs with the default value, then add a second just like it for a different option of view.

For the docs, just add a note in this file with the alternate options for views.

Thanks again!

No problem! Added the changes, let me know if all looks right. Works well on my end. 6d36b9c

@schorrm
Copy link
Collaborator

schorrm commented Dec 13, 2022

LGTM for me

@tjburch
Copy link
Collaborator

tjburch commented Dec 13, 2022

And also me. Seems pointless to run tests right now since they're all broke anyway so smashing that merge button.

Thanks a ton @zach-hopkins!

@tjburch tjburch merged commit 4c207bf into jldbc:master Dec 13, 2022
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