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

Add lm.beta tidier #545

Merged
merged 18 commits into from Dec 8, 2018
Merged

Conversation

mattle24
Copy link
Contributor

@mattle24 mattle24 commented Nov 28, 2018

TODO before merging

  • complete documentation for tidy.lm.beta
  • add tests for tidy.lm.beta
  • Add as a contributor to DESCRIPTION
  • check that code passes conventions with lintr
  • Update NEWS.md to reflect the changes you’ve made
  • check test coverage ✅
  • pass the AppVeyor and Travis CI builds ⚠️

@mattle24
Copy link
Contributor Author

From issue: #496

@mattle24
Copy link
Contributor Author

mattle24 commented Nov 28, 2018

⚠️

  • App Veyor failed because does not have lm.beta package.
  • strict tests failed because because standardized.estimate is not in the column glossary

@mattle24 mattle24 changed the title Add lm.beta tidier for tidy() Add lm.beta tidier Nov 30, 2018
@alexpghayes
Copy link
Collaborator

It looks like the build failures might clear up if you add lm.beta to suggests? You can do this with usethis::use_package("lm.beta", type = "Suggests").

@mattle24
Copy link
Contributor Author

mattle24 commented Dec 6, 2018

Sounds good, thanks! Added that so hopefully the tests will run more smoothly now.

What should I do about the tidier returning the column standardized.estimate? It's not in the glossary.
lm_beta_tidy

@alexpghayes
Copy link
Collaborator

You'll need to add it to the tidy column glossary in https://github.com/alexpghayes/modeltests. The file you'll want to change is https://github.com/alexpghayes/modeltests/blob/master/data-raw/columns_tidy.yaml

I recommending using std_estimate instead of standardized.estimate for brevity / style.

@mattle24
Copy link
Contributor Author

mattle24 commented Dec 6, 2018

Okay thanks! Changed the column name to std_estimate and opened a PR (#15) to add it to the glossary.

@mattle24
Copy link
Contributor Author

mattle24 commented Dec 6, 2018

⚠️

  • AppVeyor no longer fails because of lm.beta package
  • strict tests failed because because std_estimate is not in the column glossary

@alexpghayes
Copy link
Collaborator

Merged the PR in modeltests and am now re-running the tests.

@mattle24
Copy link
Contributor Author

mattle24 commented Dec 7, 2018

Awesome, thank you!

@mattle24
Copy link
Contributor Author

mattle24 commented Dec 8, 2018

Sorry, messed up that modeltests PR. I didn't realize I had to run create_glossaries.R. Ran that file and opened a new PR (#16) with the update. All tests pass on my system now for both test() and check().

@alexpghayes
Copy link
Collaborator

Merged the modeltests PR and restarted the build.

@alexpghayes alexpghayes merged commit 9a85f75 into tidymodels:master Dec 8, 2018
@alexpghayes
Copy link
Collaborator

Merged! Fantastic work -- thank you!

@mattle24
Copy link
Contributor Author

mattle24 commented Dec 8, 2018

Thanks for the help Alex! Really excited to be contributing!

@mattle24 mattle24 deleted the patch-lm-beta-tidier branch December 9, 2018 16:02
@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

2 participants