-
Notifications
You must be signed in to change notification settings - Fork 83
Develop #251
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
Develop #251
Conversation
Manage "infer" class more systematically
This seems to be the way to test push access :)
Use {vdiffr} for plot testing
… a few unncessary lines of code.
Details:
- Data used in those tests is now generated in 'helper-data.R' file. That is why actual {vdiffr} image outputs have changed.
This also gets to 100% coverage.
Use "area under the curve" in `shade_p_value()`
…on` synonyms (to avoid duplicating expectations).
Goodness of fit
Merge branch 'develop' of https://github.com/tidymodels/infer into develop # Conflicts: # man/t_test.Rd
Add default args to hypothesize
Merge branch 'master' into develop # Conflicts: # tests/testthat/test-wrappers.R
Suppress chisq warnings
| hypothesize_checks(x, null) | ||
|
|
||
| } | ||
| dots <- compact(list(p = p, mu = mu, med = med, sigma = sigma)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Snazzy!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's all @richierocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @richierocks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for flagging that bit about plot labels. We'll try to tack that on to the next wave of updates, led by @simonpcouch, focused on documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Looking forward to it. Happy to provide a quick review. Just tag me and I'll let you know a timeline for when I'll be able to get it done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll move it over to an issue and try to assign it to Simon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. I'm not sure why the travis build is taking so long...normally it's like a 5 min job.
ismayc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewpbray All looks good to me.
One thing I noticed that we should check for is that visualize() always labels the distribution plot as 'Null Distribution' but that isn't the case if we are creating confidence intervals for example. This is a long standing issue that I've been meaning to fix for awhile.
Codecov Report
@@ Coverage Diff @@
## master #251 +/- ##
==========================================
- Coverage 100% 98.31% -1.69%
==========================================
Files 13 15 +2
Lines 947 1068 +121
==========================================
+ Hits 947 1050 +103
- Misses 0 18 +18
|
|
@andrewpbray I'll leave this merge up to you now that I've approved it. I recommend getting the coverage back to 100% as soon as possible to prevent weird bugs from seeping through, especially since this version is up on CRAN now. |
|
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. |
This is version 0.5.0 recently accepted to CRAN.