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

Meet specification more fully (i.e. enable more strict tests) #544

Closed

Conversation

jmuhlenkamp
Copy link
Contributor

This is a pull request for #543 (also general strict tests issue #450).

Would appreciate some feedback on how I am setting the default arguments for augment.betareg as it feels a bit awkward (see below for snippet).

augment.betareg <- function(x, data = model.frame(x), newdata = NULL,
                            type.predict, type.residuals, ...) {
                            type.predict = eval(formals(betareg:::predict.betareg)$type),
                            type.residuals = eval(formals(betareg:::residuals.betareg)$type),
                            ...) {

@alexpghayes
Copy link
Collaborator

This is awesome! I'm swamped with school at the moment but will try to take a look soon!

@alexpghayes
Copy link
Collaborator

This is really really awesome! Thank you!

For augment.betareg() I would either:

  • use c("response", "link", "precision", "variance", "quantile") for type.predict
  • use c("sweighted2", "deviance", "pearson", "response", "weighted", "sweighted") for type.predict

The concern is then what happens if betareg changes these options are no longer correct. To address this you could write some tests to make sure all these options work.

Alternatively, you could use NULL for the default and then use something like:

type.predict <- match.arg(type.predict, choices = formals(betareg:::predict.betareg)$type)

in the body of the function. I think the first option is preferable, although I'm not totally sure.

@jmuhlenkamp
Copy link
Contributor Author

jmuhlenkamp commented Dec 8, 2018

When I get a chance, I'll implement the first option, add a basic test for each option, update the NEWS, and then this PR should be ready to be merged. Will let you know.

@jmuhlenkamp
Copy link
Contributor Author

This should now be ready to be merged. Let me know if there is anything I need to do.

NEWS.md Outdated Show resolved Hide resolved
R/stats-factanal-tidiers.R Outdated Show resolved Hide resolved
tests/testthat/test-betareg.R Outdated Show resolved Hide resolved
@alexpghayes
Copy link
Collaborator

Added some comments!

@jmuhlenkamp
Copy link
Contributor Author

Thanks! From a quick look, all the comments make sense and should be pretty minor changes. Will try to push updates in the next few days. Will let you know.

@jmuhlenkamp
Copy link
Contributor Author

All comments resolved via new commits. Ready for another review and/or merge.

@alexpghayes
Copy link
Collaborator

I know it's been forever, and I'm sorry! This is on my TODO list again!

@alexpghayes
Copy link
Collaborator

Looking good! Added some comments!

R/betareg-tidiers.R Show resolved Hide resolved
R/stats-factanal-tidiers.R Outdated Show resolved Hide resolved
R/stats-kmeans-tidiers.R Show resolved Hide resolved
@alexpghayes
Copy link
Collaborator

Finally followed up!

@jmuhlenkamp
Copy link
Contributor Author

Thanks! I added one comment to close out the kmeans discussion, but I should be good to make the requested change(s) now. :-)

@alexpghayes alexpghayes changed the title Default arguments strict tests Meet specification more fully (i.e. enable more strict tests) Apr 9, 2019
@jmuhlenkamp
Copy link
Contributor Author

@alexpghayes added two commits that close out the open review items. If this PR is good now, I can work to resolve the merge conflicts and/or merge master into my branch. Let me know. Also, I'm not too familiar with the CI build fails, I believe some previous changes from master that I merged into my branch are causing this, but I am not sure.

Let me know how you'd like to me proceed to keep this moving along. Thanks!

@alexpghayes
Copy link
Collaborator

Sorry that I've let this sit so long. If you resolve merge conflicts and ping me I'll make sure to review quickly.

@github-actions
Copy link

github-actions bot commented Apr 9, 2023

This pull request has been automatically closed due to inactivity.

@github-actions
Copy link

This pull request has been automatically locked. If you believe the issue addressed here persists, please file a new PR (with a reprex: https://reprex.tidyverse.org) and link to this one.

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

Successfully merging this pull request may close these issues.

None yet

3 participants