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 hypothesize() interface clearer #242

Merged
merged 5 commits into from Sep 17, 2019
Merged

Conversation

@richierocks
Copy link
Contributor

richierocks commented Aug 13, 2019

The main change is that the signature to hypothesize() is now

hypothesize <- function(x, null, p = NULL, mu = NULL, med = NULL, sigma = NULL)

This fixes #240.

Please double-check that my documentation of p, mu, med, and sigma are accurate.

The existing logic for checking arguments to hypothesize() was a bit scattered between hypothesize() itself, hypothesize_checks() and parse_params(). I did some reorganizing of code that I think makes things cleaner. I think there may be more to do here but it feels better than it was.

I also split one of the longer tests for hypothesize() up into some shorter tests for clarity and added some more. Again, it feels like there might be more work to do here in the future, but this is cleaner than it was.

@richierocks richierocks changed the title Make hypothesize() interfce clearer Make hypothesize() interface clearer Aug 13, 2019
@richierocks

This comment has been minimized.

Copy link
Contributor Author

richierocks commented Aug 13, 2019

I should probably also mention that I added a dependency on purrr for the compact() function. If you are worried about keeping infer lightweight I can remove it, but I suspect that most people using infer will have all the core tidyverse packages installed anyway, so it shouldn't be a problem.

@andrewpbray

This comment has been minimized.

Copy link
Collaborator

andrewpbray commented Aug 14, 2019

Very weird that it's not passing checks. I'm going to do a sanity check with a new branch and only a superficial change to see if we can get it past Travis.

In the meantime, I'd like to do small tweaks to your PR but don't want to bog it down as review comments; it'd be quicker just to do them myself. Do you know how I can do that in a lightweight way through GitHub instead of doing a PR into your branch?

@mine-cetinkaya-rundel

This comment has been minimized.

Copy link
Contributor

mine-cetinkaya-rundel commented Aug 15, 2019

@andrewpbray usethis::pr_*() functions will let you do that. See https://usethis.r-lib.org/articles/articles/pr-functions.html for a "worked example" of how to do that.

@andrewpbray andrewpbray merged commit de704f7 into tidymodels:develop Sep 17, 2019
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.