Take advantage of matrix column support #1126
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When the augment() documentation was first written, evidently matrix columns were not allowed in tibbles. But they have been supported since tibble version 1.0.0: tidyverse/dplyr#3630 For instance, augment.lm() works correctly for a model containing poly() or ns() terms, placing those terms in matrix columns as expected.
This was forbidden in broom 0.5.0 because tibbles did not support matrix columns, and Surv() produces matrix columns in the model.frame(). But since tibble 1.0.0, matrix columns are supported, and these work fine with the default model.frame().
|
The first CHECK failure appears to be because I didn't rebuild the Rd files, as I mentioned, not because of a test failure. |
|
Great, thank you @capnrefsmmat! Just pushed those doc updates. |
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed the documentation for
augment.lm()claimed that matrix columns are not supported and hencepoly()won't work, but I noticed this while augmenting a model with apoly()term that worked just fine. It turns out matrix column support was added in tibble 1.0.0, so matrix columns will work fine. I've updated the augment template documentation accordingly.Examining the NEWS for broom 0.5.0, I saw this also meant
augment.survreg()andaugment.coxph()required an explicit data argument, because theirmodel.frame()would include matrix columns. I verified that usingmodel.frame()works just fine now. This should not be a breaking change, because anyone who left the data argument blank before would have gotten an error, whereas now they will get a meaningful result.I have not committed the updated Rd files, because I don't have all the zillion packages supported by broom installed. But I have verified that the survival tests still pass. You're welcome to rebuild the Rd on this branch.
I'll commit a NEWS update for the
augment.survreg()andaugment.coxph()shortly. I'm already in the DESCRIPTION from #1112.