8000
Skip to content

Support for crch package - #4

Closed
carlganz wants to merge 5 commits into
leeper:masterfrom
carlganz:master
Closed

Support for crch package#4
carlganz wants to merge 5 commits into
leeper:masterfrom
carlganz:master

Conversation

@carlganz
@carlganz carlganz commented Nov 18, 2016
Copy link
Copy Markdown
Contributor

Just a small addition.

Unrelated, but I noticed that you used model.frame inside of find_data.merMod. It occurs to me now that model.frame is a generic function that essentially serves the same purpose as the generic find_data. I could be wrong, but I am pretty sure we could substitute model.frame for find_data without any problems.

@codecov-io
codecov-io commented Nov 18, 2016 •
edited
Loading
Copy link
Copy Markdown

Current coverage is 8.97% (diff: 0.00%)

Merging #4 into master will decrease coverage by 0.68%

@@            master        #4   diff @@
========================================
  Files            6         6          
  Lines          290       312    +22   
  Methods          0         0          
  Messages         0         0          
  Branches         0         0          
========================================
  Hits            28        28          
- Misses         262       284    +22   
  Partials         0         0

Powered by Codecov. Last update b7cd9f9...f466ea4

@leeper
leeper commented Nov 19, 2016
Copy link
Copy Markdown
Owner

Thanks, I'll take a look at this soon.

To answer your question, though, model.frame() is not a good generic solution. The problem is that if a model specification modifies its input variables, model.frame() will return the modified versions rather than the original versions. It is the latter (the original form) that is needed for predict() methods. See, for example:

str(model.frame(m1 <- lm(mpg ~ factor(cyl), data = mtcars)))
str(model.frame(m2 <- lm(mpg ~ I(wt*100), data = mtcars)))
predict(m1, model.frame(m1))
predict(m2, model.frame(m2))

@carlganz
carlganz commented Nov 19, 2016
Copy link
Copy Markdown
Contributor Author

I'll update this request to comply with the code of conduct. Apologies for not filing an issue first.

Thanks for the explanation. Makes sense.

@leeper
leeper commented Nov 19, 2016
Copy link
Copy Markdown
Owner

No worries at all. It looks like a good addition on quick glance.

@leeper
leeper commented Nov 20, 2016
Copy link
Copy Markdown
Owner
8000

Thanks. This looks good. I'm going to rebase and merge. You'll need to update your fork accordingly.

@leeper leeper closed this Nov 20, 2016
leeper added a commit that referenced this pull request Nov 20, 2016
@leeper
add crch and hxlr methods (#4)
00212d7
@leeper leeper mentioned this pull request Nov 20, 2016
Open
87 tasks
@carlganz
carlganz commented Nov 20, 2016
Copy link
Copy Markdown
Contributor Author

Hey, turns out model.frame won't work for crch for the reasons you mentioned above. Will fix ASAP. For some reason when I used default find_data with crch on Friday I found that if the dataset was named data it would get angry, but I can't reproduce the problem now, so I am pretty confident default find_data is fine for crch.

leeper added a commit that referenced this pull request Mar 1, 2017
@leeper
add crch and hxlr methods (#4)
e026a8d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@carlganz @codecov-io @leeper
0