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

Change names of interval columns in augment() for clarity #925

Merged
merged 10 commits into from Sep 18, 2020

Conversation

bwiernik
Copy link
Contributor

@bwiernik bwiernik commented Sep 4, 2020

Closes #921

  • Changes .conf.low to .lower and .conf.high to .upper consistently for augment() output.
  • Updates agument.rma() to consistently return one set of intervals.

@bwiernik bwiernik changed the title Interval Change names of interval columns in augment() for clarity Sep 4, 2020
R/rma-tidiers.R Outdated Show resolved Hide resolved
@simonpcouch
Copy link
Collaborator

Thanks for making this happen!

I'm feeling positive about how this came together with respect to the first bullet.

I dropped a comment in about the second.

Could you ensure that changes have been made only to relevant documentation? Quite a few .Rd files with changes only in the help-files they link out to.

…ction to .pred.low/.pred.high

The columns previously labeled "cr.lb" and "cr.ub" in rma objects are now called "pi.lb" and "pi.ub". The previous label was misleading and inaccurate--these values are not "credibility intervals" as used in the meta-analysis literature or "credible intervals" as used in Bayesian analysis. They are prediction intervals ala lm(). The change in metafor was to avoid some common confusion and misinterpretation among metafor users; this change to "pred" instead of "cred" follows suit.
@bwiernik
Copy link
Contributor Author

bwiernik commented Sep 7, 2020

I've reverted the documentation entirely.

When I try to run devtools::document(), it doesn't seem to be producing the correct output (e.g., the Value table disappears entirely for agument.rma; and the Value table for augment.lm is missing the interval arguments). I think this has to do with the return_arguments() helper function, but I'm not sure what I need to do to fix it.

(The other files being modified was that some of the .Rd files seem out of sync with the roxygen templates.)

@simonpcouch
Copy link
Collaborator

That's fine.👍

The other modified files are a result of differences in package versions. Recent switch in how documentation gets linked from place to place that can be a pain in package dev.

Only return one interval for the few metafor results that return both by default.
@simonpcouch
Copy link
Collaborator

Great, much appreciated!

There are some modeltests errors (along the lines of Output column names not in the column glossary) and some code/doc mismatch failures (since I had you roll back docs) that are fine to ignore from your end. Looks like there's still one failure in the examples that just requires some column renaming in a ggplot call--could you make that happen?

I'm feeling positive about where this is at. Looping you in, @alexpghayes.

@bwiernik
Copy link
Contributor Author

bwiernik commented Sep 8, 2020

I’ll take a look tomorrow afternoon.

@grantmcdermott grantmcdermott mentioned this pull request Sep 8, 2020
@bwiernik
Copy link
Contributor Author

bwiernik commented Sep 8, 2020

@simonpcouch The example errors are just because the documentation Rd is out of sync with the roxygen. In the roxygen, all of the ggplot calls refer to .lower and .upper, but because I didn't run document() because of the modeltestr mismatch, they are erroring.

@bwiernik
Copy link
Contributor Author

bwiernik commented Sep 9, 2020

@simonpcouch Did you want me to change anything regarding those errors or is this fine for now?

@simonpcouch
Copy link
Collaborator

Oh, gotcha! No worries on the errors, then. Thanks for checking in.

simonpcouch added a commit to alexpghayes/modeltests that referenced this pull request Sep 11, 2020
@simonpcouch
Copy link
Collaborator

Thanks for your patience!

Alex and I finally had a chance to talk on this, and we're game. Thanks for the contribution. Will go ahead and fix conflicts and then merge.

@simonpcouch simonpcouch merged commit 8bd3835 into tidymodels:master Sep 18, 2020
simonpcouch added a commit that referenced this pull request Sep 18, 2020
@bwiernik
Copy link
Contributor Author

Thanks so much! Glad it got merged!

@bwiernik bwiernik deleted the interval branch September 18, 2020 22:18
@github-actions
Copy link

github-actions bot commented Mar 6, 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 6, 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.

Column name '.pred' for prediction intervals?
2 participants