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

Make column names of output of htest-oneway tidier syntactically valid #549

Merged
merged 5 commits into from Dec 18, 2018

Conversation

karissawhiting
Copy link
Contributor

@alexpghayes - In R/stats-htest-tidiers.R there is a message indicating that columns will be renamed to num.df and denom.df if (length(x$parameter) > 1) is TRUE, however this renaming doesn't actually occur as far as I can tell (although I may be missing something).

Here is the relating section of code in the current version:

x <- oneway.test(extra ~ group, data = sleep)
ret <- x[c("estimate", "statistic", "p.value", "parameter")]

# parameter may have multiple values as well, such as oneway.test
if (length(x$parameter) > 1) {
  ret$parameter <- NULL
  if (is.null(names(x$parameter))) {
    warning("Multiple unnamed parameters in hypothesis test; dropping them")
  } else {
    message(
      "Multiple parameters; naming those columns ",
      paste(make.names(names(x$parameter)), collapse = ", ")
    )
    ret <- append(ret, x$parameter, after = 1)
  }
}
#> Multiple parameters; naming those columns num.df, denom.df
names(ret)
#> [1] NA          "num df"    "denom df"  "statistic" "p.value"

I propose the addition of names(x$parameter) <- make.names(names(x$parameter)) to make these names syntactically valid:

x <- oneway.test(extra ~ group, data = sleep)
ret <- x[c("estimate", "statistic", "p.value", "parameter")]

# parameter may have multiple values as well, such as oneway.test
if (length(x$parameter) > 1) {
  ret$parameter <- NULL
  if (is.null(names(x$parameter))) {
    warning("Multiple unnamed parameters in hypothesis test; dropping them")
  } else {
    message(
      "Multiple parameters; naming those columns ",
      paste(make.names(names(x$parameter)), collapse = ", ")
    )
    names(x$parameter) <- make.names(names(x$parameter))
    ret <- append(ret, x$parameter, after = 1)
  }
}
#> Multiple parameters; naming those columns num.df, denom.df
names(ret)
#> [1] NA          "num.df"    "denom.df"  "statistic" "p.value"

This is meant as the first step to fixing the oneway-htest tidier and glance functions (relating to issue #450) so they can pass the strict tests in tests/testthat/test-stats-htest.R. The output of stats::oneway.test currently has num df and denom df as parameters, but these are not in the column glossary.

If denom.df and num.df are added to the column glossary for tidy and glance methods after this change, these should pass the strict test (sidenote: that num.dfis already in the column glossary for the tidy method).

To finish this strict test issue fix, I propose these follow up steps (after this PR is merged).

  1. Make a PR on modeltests with a change on the column_glossary.rda file binding the following additions:
 additions <- tribble(
  ~method, ~column, ~description, 
  "tidy", "denom.df", "Denominator degrees of freedom.", 
  "glance", "num.df", "Degrees of freedom.",
  "glance", "denom.df", "Denominator degrees of freedom.") 
  1. After that change is merged, make an additional PR here changing strict = TRUE in the check_tidy_outputs and check_glance_outputs functions for the oneway tests of tests/testthat/test-stats-htest.R.

Please let me know if you agree with this plan. Thanks!

@IndrajeetPatil
Copy link
Contributor

I think this PR solves #500

@alexpghayes
Copy link
Collaborator

alexpghayes commented Dec 8, 2018

Looks good! Will you

  • Add a quick comment at the spot I mentioned in the review
  • Add a test that num df and denom df don't appear in output anymore
  • Update NEWS
  • Add yourself as a contributor in DESCRIPTION

and then I'll merge?

…md and DESCRIPTION, add comment to document change
@karissawhiting
Copy link
Contributor Author

@alexpghayes - Let me know if you had something else in mind for tests regarding num df and denom df in the output. Thanks!

tests/testthat/test-stats-htest.R Outdated Show resolved Hide resolved
tests/testthat/test-stats-htest.R Outdated Show resolved Hide resolved
@alexpghayes
Copy link
Collaborator

Looks good! Made a small suggestion, but we're close to merging!

alexpghayes and others added 2 commits Dec 18, 2018
Co-Authored-By: karissawhiting <karissa.whiting@gmail.com>
Co-Authored-By: karissawhiting <karissa.whiting@gmail.com>
@alexpghayes alexpghayes merged commit 39b9f44 into tidymodels:master Dec 18, 2018
@alexpghayes
Copy link
Collaborator

Awesome! Thanks for contributing!

@github-actions
Copy link

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 10, 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