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

scrambles for older years #468

Merged
merged 14 commits into from
Jun 26, 2024
Merged

scrambles for older years #468

merged 14 commits into from
Jun 26, 2024

Conversation

guga31bb
Copy link
Member

@guga31bb guga31bb commented Jun 13, 2024

Still a work in progress. The older years of data are a mess in the NFL pbp

Copy link

github-actions bot commented Jun 13, 2024

@guga31bb guga31bb requested a review from mrcaseb June 14, 2024 13:02
@guga31bb
Copy link
Member Author

@mrcaseb I think this is ready except for the test failing because it changes the output and no longer matches expected output (eg scramble, rush, pass etc change on some plays)

R/helper_add_nflscrapr_mutations.R Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@mrcaseb
Copy link
Member

mrcaseb commented Jun 14, 2024

I can update test expectation but will be out of town until Sunday

guga31bb and others added 3 commits June 14, 2024 09:32
because some scrambles changed with this PR
@mrcaseb mrcaseb self-requested a review June 17, 2024 07:29
mrcaseb
mrcaseb previously approved these changes Jun 17, 2024
@mrcaseb mrcaseb enabled auto-merge (rebase) June 17, 2024 07:29
@mrcaseb
Copy link
Member

mrcaseb commented Jun 17, 2024

Seems like MacOS isn't happy with the new expectation. I will review later.

Also I introduced some new "global variable" warnings that could be fixed in this PR as well

if unix/windows fail now, we have to round to 3 significant digits instead of 4
@guga31bb
Copy link
Member Author

Tests still failing?

@tanho63
Copy link
Member

tanho63 commented Jun 24, 2024

image
rounding error, looks like. I would try adding this tolerance param to expect_equal and set it to ~ 0.00001 or so

@guga31bb
Copy link
Member Author

Still fails even with a pretty lenient tolerance. I am confused. It's like it's ignoring the tolerance param.

it wasn't saved correctly anyways as the initial "round_double_to_digits" converted all double variables to character and I saved this as expectation
also round to 3 significant digits because tests failed on Mac in the 4th digit
@mrcaseb mrcaseb merged commit d4c5c43 into master Jun 26, 2024
6 checks passed
@mrcaseb mrcaseb deleted the old_scrambles branch June 26, 2024 10:08
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

3 participants