-
Notifications
You must be signed in to change notification settings - Fork 80
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
add z argument to prop_test #353
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
need the new z functionality from #351
allows the user to report a standard normal deviate rather than a chi-square statistic. also enables two-sample z statistic calculation after having hypothesized and introduces a non-breaking `correct` argument to avoid unnecessarily complex logic regarding `...`.
Beautiful! Thanks much and happy holidays! |
ismayc
approved these changes
Dec 22, 2020
echasnovski
reviewed
Dec 24, 2020
echasnovski
reviewed
Dec 24, 2020
Nice work! Thank you. |
This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[EDIT: fixed visual clutter of diffs!]
Here’s a go at implementing the
z
argument forprop_test()
to report thez
statistic instead of the chi-square statistic!I opted to fully make use of the infer observed statistic pipeline rather than capitalizing on the connection between the standard normal and chi-square to calculate the statistic. Otherwise, the output piggybacks off of the current output, switching out the statistic, removing the
chisq_df
column, and leaving the rest of the columns.This implementation also turns off the Yates continuity correction when
z = TRUE
so that p-values and intervals align with manual calculations as expected. It also introduces a non-breakingcorrect
argument that was previously passed through...
to avoid introducing weird internal logic to overwrite components of the ellipses that were modified elsewhere.This PR also enables the following code, which used to return the specified
gss
as is:…to give the same output as this code:
Relatedly, I agree with Evgeni’s comment on my last PR that
calculate()
deserves a refactor. Probably worth specifying more strictly whatcalculate()
s behavior should be in calculating observed test statistics—I’ll open a PR/issue related to this after the holidays! Namely for this PR, I think deleting thegenerate()
line in an infer pipeline and otherwise leaving code as is should report the appropriate observed statistic. Ifhypothesize()
is omitted when it probably ought not to be (for reporting point estimates), assume a reasonable null value and report it as a warning.Created on 2020-12-22 by the reprex package (v0.3.0.9001)
Closes #347.