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

Supply ff_scoring() as an argument to ff_scoringhistory() to allow for customization #375

Open
TheMathNinja opened this issue Aug 17, 2022 · 14 comments
Labels
feature 🌟 a feature request or enhancement

Comments

@TheMathNinja
Copy link
Contributor

I think this should be pretty straightforward, as there's already stat mapping for this. I like keeping fumbles in there, but most fantasy scoring systems track fumbles lost instead. Could you add rushing_fumbles_lost, receiving_fumbles_lost, and sack_fumbles_lost to this function? Thanks!

@tanho63 tanho63 added the feature 🌟 a feature request or enhancement label Aug 17, 2022
@tanho63
Copy link
Member

tanho63 commented Aug 17, 2022

PR welcome if you like, unsure when I'll get to this - probably not in the next release

@TheMathNinja
Copy link
Contributor Author

I suck at PR's (doing it "right" is like 9 steps I don't remember how to do, haha). I can try, but I'm having trouble finding the code for ff_playerscores(). Any tips on how to navigate this repository to find it?

@tanho63
Copy link
Member

tanho63 commented Aug 17, 2022

@TheMathNinja
Copy link
Contributor Author

That needs no updates. Fumbles lost are already in there. They just aren't showing up in the df output of ff_scoringhistory(). That's what I meant when I said there was already stat mapping for this. So it's just the function itself that needs an update, right?

@tanho63
Copy link
Member

tanho63 commented Aug 17, 2022

No, you'll need to add fumbles_lost for MFL. Also, nflverse only has total fumbles lost, not split into rushing/receiving etc. This may cause double-counting of fumbles lost, which is why it's not there in the first place iirc

edit: I lied, it just needs the mapping.

@TheMathNinja
Copy link
Contributor Author

I’m still confused because the mapping here includes all 3 types of fumbles lost for MFL.

@tanho63
Copy link
Member

tanho63 commented Aug 17, 2022

Okay, I was interpreting based on your question but auditing an actual test case - fumbles lost is already on here:

ffscrapr::ff_scoringhistory(ffscrapr::mfl_connect(2022,54040)) |> names()
 [1] "season"                    "week"                      "gsis_id"                  
 [4] "sportradar_id"             "mfl_id"                    "player_name"              
 [7] "pos"                       "team"                      "points"                   
[10] "interceptions"             "passing_2pt_conversions"   "passing_tds"              
[13] "passing_yards"             "receiving_2pt_conversions" "receiving_fumbles_lost"   
[16] "receiving_tds"             "receiving_yards"           "receptions"               
[19] "rushing_2pt_conversions"   "rushing_fumbles_lost"      "rushing_tds"              
[22] "rushing_yards"             "sack_fumbles_lost"         "special_teams_tds"        
[25] "rushing_first_downs"       "receiving_first_downs"   

So what bug are you looking at?

@TheMathNinja
Copy link
Contributor Author

Here's what I'm getting:

names(ffscrapr::ff_scoringhistory(mfl_connect(season = 2021, league_id = 22686, rate_limit_number = 3, rate_limit_seconds = 6), 
season = 2019:2021))

 [1] "season"                    "week"                     
 [3] "gsis_id"                   "sportradar_id"            
 [5] "mfl_id"                    "player_name"              
 [7] "pos"                       "team"                     
 [9] "points"                    "attempts"                 
[11] "carries"                   "completions"              
[13] "interceptions"             "passing_2pt_conversions"  
[15] "passing_first_downs"       "passing_tds"              
[17] "passing_yards"             "receiving_2pt_conversions"
[19] "receiving_first_downs"     "receiving_fumbles"        
[21] "receiving_tds"             "receiving_yards"          
[23] "receptions"                "rushing_2pt_conversions"  
[25] "rushing_first_downs"       "rushing_fumbles"          
[27] "rushing_tds"               "rushing_yards"            
[29] "sack_fumbles"              "sack_yards"               
[31] "sacks"                     "targets"         

@TheMathNinja
Copy link
Contributor Author

OH WAIT! Is ff_scoringhistory() written so that it only includes variables that are used in your particular league's scoring system? That explains to me what's going on here. What do you think about updating the function so that it imports all variables available on the platform, but only computes fantasy points based on the relevant ones? I think this would be extremely helpful for commissioners who want to play around with seeing how using new stats would affect player scores in their leagues. I guess this is my new/official feature request if I'm right about what's happening.

@tanho63
Copy link
Member

tanho63 commented Aug 17, 2022

Better would be adding an optional argument that would provide an ff_scoring dataframe to use instead of the original. Busy, but can stick onto the backlog

@tanho63 tanho63 changed the title Add fumbles_lost to ff_scoringhistory() Supply ff_scoring() as an argument to ff_scoringhistory() to allow for customization Aug 17, 2022
@TheMathNinja
Copy link
Contributor Author

TheMathNinja commented Aug 17, 2022

Better would be adding an optional argument that would provide an ff_scoring dataframe to use instead of the original. Busy, but can stick onto the backlog

Ooo yeah, good point. So the argument would be something like "customize = TRUE" if we want to get just the variables currently present/counted in our scoring system? And currently this is set to TRUE by default, but in the update it would be set to FALSE?

@tanho63
Copy link
Member

tanho63 commented Aug 17, 2022

it would be something like

ff_scoringhistory(conn, scoring = ff_scoring(conn))

by default, and you can supply it a different dataframe if you want different scoring, I think. Not sure yet but I feel that would be the best implementation

@TheMathNinja
Copy link
Contributor Author

Would that argument affect the points variable and how it’s calculated, the variables passed to the output df, or both? And if it’s left blank, what happens? I guess I’m still wanting to make sure that there’s a way I can pass an MFL conn object that uses, say, only 10 of MFL’s available stats in my scoring, but still see the output of all available MFL stats whether I use it or not (ideally by default).

@tanho63
Copy link
Member

tanho63 commented Aug 17, 2022

It's a performance consideration and slows down an already slow query, so no - it would only return scoring history for the scoring variables that are passed into the function. The right thing to do would be to have a different function or parameter in ff_scoring() that shows all rules. Then your workflow would be:

  • run ff_scoring() and save into an object
  • pass this object to ff_scoringhistory()
  • edit/update the ff_scoring object (possibly adding a new scoring setting, per mfl_allrules()) and pass the revised object to ff_scoringhistory, then compare the output of the two functions as desired

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🌟 a feature request or enhancement
Projects
None yet
Development

No branches or pull requests

2 participants