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

Reduce code duplication #177

Merged
merged 12 commits into from Aug 14, 2018
Merged

Reduce code duplication #177

merged 12 commits into from Aug 14, 2018

Conversation

echasnovski
Copy link
Collaborator

This is a PR for #173. It contains updates suggested in issue with major refactor of visualize(). It also has the following user-visible changes:

  • Now all shading of p-value tails and intervals is done with the same transparency. In current 'master' version transparency is fixed for method = "theoretical", but varies for method = "simulation" or method = "both". This is because of actually different implementations of shading. I didn't consult if this is an intentional behaviour, but it seems unlikely to me. To see an effect, run the following code:
library(infer)
plot_iris_boot <- function(reps) {
  set.seed(101)
  
  iris_boot <- iris %>%
    tibble::as_tibble() %>%
    dplyr::mutate(
      Sepal.Length.Group = dplyr::if_else(Sepal.Length > 5, ">5", "<=5"),
      Sepal.Width.Group = dplyr::if_else(Sepal.Width > 3, "large", "small")
    ) %>%
    specify(Sepal.Length ~ Species) %>%
    hypothesize(null = "independence") %>%
    generate(reps = reps, type = "permute") %>%
    calculate(stat = "F") %>%
    visualize(method = "both", obs_stat = 2, direction = "two_sided") %>%
    print()
}

plot_iris_boot(20)
plot_iris_boot(100)
  • Now visualize() works for "One sample t" theoretical distribution type for method = "both". It didn't work because it was missing in this if clause. After wrapping these checks into a function, this became apparent. To see that it wasn't working, run the following code:
library(infer)
iris %>%
  tibble::as_tibble() %>%
  dplyr::mutate(
    Sepal.Length.Group = dplyr::if_else(Sepal.Length > 5, ">5", "<=5"),
    Sepal.Width.Group = dplyr::if_else(Sepal.Width > 3, "large", "small")
  ) %>%
  specify(Petal.Width ~ NULL) %>%
  hypothesize(null = "point", mu = 1) %>%
  generate(reps = 100) %>%
  calculate(stat = "t") %>%
  visualize(method = "both")

@echasnovski
Copy link
Collaborator Author

Oh, and also in warnings about "Chi-Square" not being good for direction other than "greater" or "right" distribution is named "Chi-Square" instead of "Chi-square" for consistency.

@echasnovski
Copy link
Collaborator Author

Travis error seems to not be code fault (locally and on other R versions it passes all checks). Error comes from pandoc:
The command "curl -fLo /tmp/pandoc-2.2-1-amd64.deb https://github.com/jgm/pandoc/releases/download/2.2/pandoc-2.2-1-amd64.deb" failed and exited with 7 during .

@codecov-io
Copy link

Codecov Report

Merging #177 into develop will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #177    +/-   ##
=======================================
  Coverage      100%   100%            
=======================================
  Files           12     12            
  Lines         1290   1135   -155     
=======================================
- Hits          1290   1135   -155
Impacted Files Coverage Δ
R/p_value.R 100% <100%> (ø) ⬆️
R/set_params.R 100% <100%> (ø) ⬆️
R/visualize.R 100% <100%> (ø) ⬆️
R/calculate.R 100% <100%> (ø) ⬆️
R/generate.R 100% <100%> (ø) ⬆️
R/specify.R 100% <100%> (ø) ⬆️
R/utils.R 100% <100%> (ø) ⬆️

Copy link
Collaborator

@ismayc ismayc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff! Thanks much!

@ismayc
Copy link
Collaborator

ismayc commented Aug 13, 2018

@echasnovski I'm stepping away from the computer now so I'm not able to merge this. I just merged in a way to add visualise() as an alias and I suspect that's the reason for this problem. Can you resolve the conflicts so I can merge?

Merge branch 'develop' into dry-code

# Conflicts:
#	R/visualize.R
@ismayc ismayc merged commit 81402ba into tidymodels:develop Aug 14, 2018
@echasnovski echasnovski deleted the dry-code branch October 15, 2018 13:00
@github-actions
Copy link

github-actions bot commented Mar 9, 2021

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.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants