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

fix issue #567 (ergm), fix issue #572 (svyglm) #611

Merged
merged 16 commits into from Apr 5, 2019
Merged

fix issue #567 (ergm), fix issue #572 (svyglm) #611

merged 16 commits into from Apr 5, 2019

Conversation

briatte
Copy link
Contributor

@briatte briatte commented Feb 27, 2019

Fix issue #567 (warnings with ergm objects)

The code now calls the ergm:::summary.ergm function internally, to avoid a warning about the function not being meant to be called directly.

Note that a problem will subsist with ergm::get.miss.dyads (used in the glance method), which is deprecated. ergm::get.miss.dyads has been replaced with the code from ergm 3.9.4. Note sure it's doing the right thing, though: see my discussion of the issue in #567. Fixed thanks to @krivit.

Fix issue #572 (glance method not working for svyglm objects)

The code adds tidy.svyglm and glance.svyglm. The first one is just a copy of close to tidy.(g)lm. The second one is identical to glance.glm, except:

  • The survey:::summary.svyglm function is called instead of summary (which evaluates to stats::summary.glm).

  • The BIC is computed differently, using survey:::dBIC(model, maximal = model) (a maximal model can be provided, but the default is to use the same model).

  • No logLik is returned, given that svyglm is not fitted via ML.

I have added an example for glance.svyglm, provided by @IndrajeetPatil in #572.

Side comment: BIC in glm and svyglm

The docs for glance.glm and glance.svyglm (in @evalRd return_glance()) say that they return the BIC, but they do not always do so:

library(survey)
data(api)
m <- glm(formula = sch.wide ~ ell + meals + mobility, data = apistrat, 
         family = quasibinomial())
AIC(m)
BIC(m)
logLik(m)
glance(m)

(Comments below made obsolete by changes sollicited by @alexpghayes in the comments below.)

The biggest change is that I had to add a dirty hack to finish_glance in utilities.R in order to extract the AIC from a svyglm object, since the result of survey:::AIC.svyglm is not similar to that of stats::AIC (the first one is a 3-element named vector, the second one is a 1-element vector).

A similar hack was already in the code (for the deviance of REML objects created by lmerMod).

note that a problem will subsist with  ergm::get.miss.dyads, which is deprecated
@briatte briatte changed the title avoid warnings mentioned in issue #567 fix issue #567 (ergm), fix issue #572 (svyglm) Feb 28, 2019
R/survey-tidiers.R Outdated Show resolved Hide resolved
R/survey-tidiers.R Show resolved Hide resolved
R/survey-tidiers.R Outdated Show resolved Hide resolved
@alexpghayes
Copy link
Collaborator

Code is very clean and I love the tests! Left some comments -- am trying to make broom more modular and have some suggestions to that extent.

@briatte
Copy link
Contributor Author

briatte commented Mar 9, 2019

@alexpghayes Hi -- I have implemented your suggestions.

Quick question -- would you be interested in removing some dependencies to the package? I'm pretty sure that I can remove stringr (by using base grepl and the like) and reshape2 (by using only tidyr, although perhaps that's a bit risky since reshape2 is probably more stable than tidyr).

Update -- I've explored both options above, and while removing stringr is doable, it'd make the code much less readable. As for reshape2, as far as I understand, reshape2::melt and tidyr::gather are not identical enough for replacing all calls to the former with the latter. I never really understood that.

R/survey-tidiers.R Outdated Show resolved Hide resolved
R/survey-tidiers.R Outdated Show resolved Hide resolved
R/survey-tidiers.R Outdated Show resolved Hide resolved
@alexpghayes
Copy link
Collaborator

Few more comments.

I'm planning on looking at dependencies more systematically in #582 at some point. Personally I find stringr much more readable than base R string manipulation and would actually prefer to move to it, rather than away for it. Same for tidyr vs reshape(), although there's one or two places where we use reshape on 3-arrays that I don't think we can get rid of. So: yes, dependencies are my mind, but cleaning up the code base is a higher priority at the moment.

@briatte
Copy link
Contributor Author

briatte commented Mar 10, 2019

Hi @alexpghayes

All suggested changes implemented, although let me know if you want glance.svyglm to lose all the comments, which I have left for clarity but which can be removed if need be.

Not sure why the PR creates a conflict.

@briatte
Copy link
Contributor Author

briatte commented Mar 24, 2019

Removed conflicts with base branch.

@briatte
Copy link
Contributor Author

briatte commented Mar 26, 2019

Improved code for ergm thanks to @krivit's messages in #567.

@alexpghayes
Copy link
Collaborator

Thank you so much!

@briatte
Copy link
Contributor Author

briatte commented Mar 28, 2019

You're welcome. I like working on broom as a way to get into the details of the models that I use.

I think it'd be a good idea for me to stop adding to this PR for now, and to come back to it if ergm 3.10 introduces other changes.

In the meantime, I'll have a look at how to augment svyglm.

@alexpghayes alexpghayes merged commit 1dd8552 into tidymodels:master Apr 5, 2019
@alexpghayes
Copy link
Collaborator

Thank you both for taking care of this and your patience!

@github-actions
Copy link

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

None yet

2 participants