created load_injuries

[john-b-edwards]

• created function to load injury report data from nflfastr-roster, load_injuries
• documented load_injuries
• added tests for load_injuries

[tanho63]

Whoa, thanks for the PR @john-b-edwards! Can you bump the DESCRIPTION version to v0.0.5, add the function to the pkgdown function listing, and (optionally) add yourself as a ctb? I'll run the tests now

[john-b-edwards]

Sure thing!

[tanho63]

Leaving for Seb to skim tomorrow

[mrcaseb]

I think I'd prefer the download logic of load_player_stats() here where we download the combined qs file and filter out the relevant season instead of using map_dfr because binding the rows can be inefficient compared to downloading the combined file.

[mrcaseb]

And we should add a variables vignette at some point.

[john-b-edwards]

It's more efficient in some cases but less efficient in others. For example, with just one season, the map_dfr logic runs 3-4x faster:

> microbenchmark(load_depth_charts_map_dfr(2020),
+                load_depth_charts_qs(2020))
Unit: milliseconds
                            expr      min       lq     mean   median       uq      max neval
 load_depth_charts_map_dfr(2020) 144.6880 150.3648 158.3617 153.5149 159.7043 269.7690   100
      load_depth_charts_qs(2020) 413.3846 434.7697 462.5940 449.0656 464.7591 607.2297   100

but above a certain number of seasons, downloading the combined qs becomes faster.

> microbenchmark(load_depth_charts_map_dfr(2010:2020),
+                load_depth_charts_qs(2010:2020))
Unit: milliseconds
                                 expr       min        lq      mean    median       uq       max neval
 load_depth_charts_map_dfr(2010:2020) 1667.4698 1733.1402 1832.0657 1774.8827 1854.064 3801.4386   100
      load_depth_charts_qs(2010:2020)  425.5798  467.1775  509.8121  487.0074  534.993  688.6993   100

Happy to convert to the qs logic but curious which use case will be more prevalent or if there's a way to grant us some flexibility with this.

[mrcaseb]

Oh yeah I assumed depth charts don't make sense for this logic as the overall file is quite big compared to one season. But that's not necessarily true for injuries (which is the reason why I commented this in this PR)?

[tanho63]

I’d just weigh in for these that I’d probably estimate the odds of someone wanting all the seasons vs one season in specific. Injuries perhaps all, depth chart perhaps one or two seasons? 🤷‍♂️

[mrcaseb]

I tend to do season level for depth charts and overall for injuries.

[john-b-edwards]

🤦 this is what i get for trying to work on two PRs simultaneously at 1 AM. Sorry @mrcaseb for the confusion

[john-b-edwards]

Total agreement here, I think what you suggested works great. I'll put them together and push in a bit

[tanho63]

Realized also this morning that it should probably be added to the memoise and cache clearing sections also - Tan Ho

…

________________________________ From: john-b-edwards ***@***.***> Sent: Thursday, August 5, 2021 12:11:10 PM To: nflverse/nflreadr ***@***.***> Cc: Tan Ho ***@***.***>; Comment ***@***.***> Subject: Re: [nflverse/nflreadr] created load_injuries (#14) Total agreement here, I think what you suggested works great. I'll put them together and push in a bit — You are receiving this because you commented. Reply to this email directly, view it on GitHub<#14 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AJCRZ33WJ7NQNNCFNKF6P5DT3KZZ5ANCNFSM5BSIRZQQ>. Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>.

[mrcaseb]

Looks good to me. Ready to merge @tanho63?