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

Update to use g_mlm() #33

Merged
merged 32 commits into from
May 22, 2020
Merged

Update to use g_mlm() #33

merged 32 commits into from
May 22, 2020

Conversation

manchen07
Copy link
Collaborator

  • Create a new branch as this is an extensive change.
    I used a previous branch: updating-to-g_mlm.

  • Import g_mlm() from lmeInfo.
    I put lmeInfo in the Depends field in DESCRIPTION and used importFrom g_mlm(), extract_varcomp(), varcomp_vcov(), and Fisher_info(). I tried moving lmeInfo to the Imports field but the Laski_g_mlm example in confidence-interval-functions.R returns error that g_mlm can not be found.

  • Import CI_g() from lmeInfo and remove scdhlm::CI_g(). Note that we'll first need to make sure that lmeInfo::CI_g() works with g_REML objects.
    I first tried to Defunct scdhlm::CI_g() and import lmeInfo::CI_g(), but CI_g() still calls the function scdhlm::CI_g(). And I am not sure how to make lmeInfo::CI_g() works with g_REML objects. If we do not update CI_g() in lmeInfo, I think the easiest way to update scdhlm::CI_g() is to rewrite the function (this is what I did).

  • Update the scdhlm vignette to use g_mlm().
    Done. But g_mlm() and the inline code are not evaluated now because scdhlm is not updated yet. After this branch gets merged to master branch, I will reinstall scdhlm from Github and then re-knit the Rmd. Should I add confidence intervals for all the models in the vignette?

  • Update the README to use g_mlm().
    Done. Same with the vignette that g_mlm(), CI_g() and the inline code are not evaluated.

  • Edit documentation to deprecate g_REML().
    Added documentation for the deprecated and removed functions.

  • Update the shiny app to use g_mlm().
    Need more time on this.

I am not sure about importing g_mlm() and CI_g(). Please let me know if you would like to do this differently. Thanks!

@manchen07 manchen07 requested a review from jepusto May 20, 2020 11:06
@manchen07 manchen07 linked an issue May 20, 2020 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented May 20, 2020

Codecov Report

Merging #33 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #33   +/-   ##
=======================================
  Coverage   52.81%   52.81%           
=======================================
  Files           7        7           
  Lines         640      640           
=======================================
  Hits          338      338           
  Misses        302      302           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56218b5...56218b5. Read the comment docs.

@manchen07
Copy link
Collaborator Author

Update the shiny app to use g_mlm().

  • In lme-fit.R, I updated lme_fit_MB() and effect_size_RML() with g_mlm().
  1. The functions can handle 3 level or 4 level data now, which includes case/series/within, cluster/case/within, and cluster/case/series/within. I do not know how to update the functions so that they can handle data with arbitrary levels.
  2. I set infotype = "expected in g_mlm() for now. Might need to update the UI if we allow specifying infotype.
  • In helper-functions.R, I updated summarize_ES().
  1. The rho is not calculated correctly now. I am not sure how to calculate and display rho for models with more than 2 levels.
  2. Should we allow the users to specify symmetric or asymmetric for CI_g()? I chose asymmetric so that the result is consistent with the published shiny app.
  3. Should we add cor_params and var_params estimates to the summary table? For example, if a user specifies a multivariate model with corSymm correlation structure, should we report this information in the summary table?

Sorry for so many questions...Please consider this whole pull request as a draft :) Just wanted to show you what I have done so far. Thank you!

Copy link
Owner

@jepusto jepusto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I've left several comments on some pieces. We can talk through the comments tomorrow. The only major comment I have is on the changes to the shiny app lme-fit functions. Let's talk about your approach here to make sure we're on the same page.

I am getting errors when I use the shiny app to calculate effect sizes for certain models right now. For instance, try using the Schutte data in a model with random intercept & slope in baseline and a fixed slope in treatment (or pretty much any other model).

R/REML-ES-functions.R Outdated Show resolved Hide resolved
R/REML-ES-functions.R Outdated Show resolved Hide resolved
R/REML-ES-functions.R Outdated Show resolved Hide resolved
R/REML-ES-functions.R Outdated Show resolved Hide resolved
R/REML-ES-functions.R Outdated Show resolved Hide resolved
R/REML-ES-functions.R Outdated Show resolved Hide resolved
README.Rmd Outdated Show resolved Hide resolved
R/confidence-interval-functions.R Outdated Show resolved Hide resolved
inst/shiny-examples/scdhlm/helper-functions.R Show resolved Hide resolved
@jepusto
Copy link
Owner

jepusto commented May 21, 2020

I made some minor changes to the README and vignette, so be sure to re-pull.

@jepusto
Copy link
Owner

jepusto commented May 21, 2020 via email

@jepusto
Copy link
Owner

jepusto commented May 21, 2020

Hi Man, I think I figured out the namespace issues. The trick is that we have to import the functions from lmeInfo, but then also re-export them from scdhlm so that they can be called by a user who has loaded the package (but hasn't loaded lmeInfo). But then if we export the functions, they also have to be documented, so I added a very simple set of Roxygen documentation. See the file R/lmeInfo-imports.R.

@manchen07
Copy link
Collaborator Author

manchen07 commented May 21, 2020 via email

@manchen07
Copy link
Collaborator Author

I am getting errors when I use the shiny app to calculate effect sizes for certain models right now. For instance, try using the Schutte data in a model with random intercept & slope in baseline and a fixed slope in treatment (or pretty much any other model).

Was the error "object fixed not found"? I got this error. Then I added X_design and Z_design back and it seems that shiny app is working. I do not understand why we need them for g_mlm(). Please let me know if you get a different error. I can debug more.

@manchen07
Copy link
Collaborator Author

I am not sure why Travis CI build failed...

@jepusto
Copy link
Owner

jepusto commented May 22, 2020

Do we need to export extract_varcomp_ex()? I don't see why anyone would need the function, and it is not exported in the master branch.

@jepusto
Copy link
Owner

jepusto commented May 22, 2020

I am getting errors when I use the shiny app to calculate effect sizes for certain models right now. For instance, try using the Schutte data in a model with random intercept & slope in baseline and a fixed slope in treatment (or pretty much any other model).

Was the error "object fixed not found"? I got this error. Then I added X_design and Z_design back and it seems that shiny app is working. I do not understand why we need them for g_mlm(). Please let me know if you get a different error. I can debug more.

Yes, the error was "object fixed not found". Even with X_design and Z_design back in, the app is still throwing the error. It's also erroring if you choose moment estimation in the Modeling tab.

@jepusto
Copy link
Owner

jepusto commented May 22, 2020

The bug in the shiny app seems to be a problem with the call to model_matrix(mod, data = nlme::getData(mod)), which happens in lmeInfo::Fisher_info(). There seems to be a problem with how model_matrix() evaluates the formula from mod. Not quite sure what to do about it at the moment so I'm gonna go to bed.

@manchen07
Copy link
Collaborator Author

manchen07 commented May 22, 2020 via email

@manchen07
Copy link
Collaborator Author

I am getting errors when I use the shiny app to calculate effect sizes for certain models right now. For instance, try using the Schutte data in a model with random intercept & slope in baseline and a fixed slope in treatment (or pretty much any other model).

Was the error "object fixed not found"? I got this error. Then I added X_design and Z_design back and it seems that shiny app is working. I do not understand why we need them for g_mlm(). Please let me know if you get a different error. I can debug more.

Yes, the error was "object fixed not found". Even with X_design and Z_design back in, the app is still throwing the error. It's also erroring if you choose moment estimation in the Modeling tab.

I tried to fix the bugs.

  • Regarding the error "object fixed not found", in lme-fit.R, mod$call$fixed was the word "fixed" and mod$call$random was "random". I think this is why model_matrix() could not evaluate the formula from mod. So I add the fixed and random formula to mod now.
  • Regarding the error with moment estimation, that is because effect_size_ABK() and effect_size_MB() save the autocorrelation as "phi" not "cor_params".

I am not sure if the errors are fixed although it seems work on my end. Please let me know how it goes.

@jepusto
Copy link
Owner

jepusto commented May 22, 2020 via email

Merge branch 'updating-to-g_mlm' of http://github.com/jepusto/scdhlm into updating-to-g_mlm

# Conflicts:
#	R/confidence-interval-functions.R
#	man/CI_g.Rd
@jepusto
Copy link
Owner

jepusto commented May 22, 2020

Okay I think we've got the CI_g issues resolved. Can you think of anything else outstanding? If not, I'll go ahead and merge with the master branch.

@manchen07
Copy link
Collaborator Author

Okay I think we've got the CI_g issues resolved. Can you think of anything else outstanding? If not, I'll go ahead and merge with the master branch.

Yes, please merge! We do need to reinstall scdhlm from Github and update README (I can do this). Not sure if the pkgdown will work but we will see. Thanks!

@jepusto jepusto merged commit eb961e1 into master May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to use g_mlm()
3 participants