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

report choleskys in tmb #269

Merged
merged 23 commits into from
May 18, 2023
Merged

report choleskys in tmb #269

merged 23 commits into from
May 18, 2023

Conversation

clarkliming
Copy link
Collaborator

close #263

minimum changes to the current code to unblock other functionalities.

@danielinteractive danielinteractive self-assigned this May 16, 2023
Copy link
Collaborator

@danielinteractive danielinteractive left a comment

Choose a reason for hiding this comment

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

Thanks Liming, general approach is good! Can we please avoid the duplicate code, by factoring out the common part of get_chol and fit_mmrm into helper function?
Also, please consider keeping get_chol as internal helper function. I don't think we would want to call this function as a user?

R/get_chol.R Outdated Show resolved Hide resolved
R/get_chol.R Outdated Show resolved Hide resolved
R/get_chol.R Outdated Show resolved Hide resolved
tests/testthat/test-get_chol.R Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented May 16, 2023

Unit Tests Summary

       1 files       40 suites   15s ⏱️
   362 tests    323 ✔️ 39 💤 0
1 769 runs  1 728 ✔️ 41 💤 0

Results for commit 9764c69.

♻️ This comment has been updated with latest results.

@clarkliming
Copy link
Collaborator Author

thank you @danielinteractive for the comment. this PR was not finalized yet yesterday due to limited time, the PR is created just to say that I am working on it and has some progress to enable github action checks. Now it should be fine and ready for review.

For avoiding duplicated code, I think the majority of the code can not be reduced. A lot of code is to check the consistency between the new data and old data. Only thing duplicated is the TMB::MakeADFun but this is super short.

R/get_chol.R Outdated Show resolved Hide resolved
R/get_chol.R Outdated Show resolved Hide resolved
R/get_chol.R Outdated Show resolved Hide resolved
R/get_chol.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@danielinteractive danielinteractive left a comment

Choose a reason for hiding this comment

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

Thanks Liming!
Yeah no worries, I just reviewed already yesterday because you requested review and it was not draft PR :-)
Regarding the file location, how about moving this to tmb.R?

R/get_chol.R Outdated Show resolved Hide resolved
R/get_chol.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/get_chol.R Outdated Show resolved Hide resolved
R/utils.R Show resolved Hide resolved
tests/testthat/test-get_chol.R Outdated Show resolved Hide resolved
tests/testthat/test-utils.R Outdated Show resolved Hide resolved
clarkliming and others added 3 commits May 17, 2023 16:30
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
@clarkliming
Copy link
Collaborator Author

very interestingly that the trigger of review seems to be automatic (even if you do not include anyone as reviewer)

Copy link
Collaborator

@danielinteractive danielinteractive left a comment

Choose a reason for hiding this comment

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

Thanks Liming, looks nice! Only few minor last suggestions, afterwards good to merge

R/tmb.R Outdated Show resolved Hide resolved
R/tmb.R Outdated Show resolved Hide resolved
R/tmb.R Outdated Show resolved Hide resolved
R/tmb.R Outdated Show resolved Hide resolved
R/tmb.R Outdated Show resolved Hide resolved
R/tmb.R Outdated Show resolved Hide resolved
tests/testthat/test-tmb.R Outdated Show resolved Hide resolved
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
clarkliming and others added 8 commits May 18, 2023 14:39
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
Co-authored-by: Daniel Sabanes Bove <danielinteractive@users.noreply.github.com>
@clarkliming clarkliming enabled auto-merge (squash) May 18, 2023 12:47
@clarkliming clarkliming merged commit 5a61959 into main May 18, 2023
@clarkliming clarkliming deleted the 263_chol_report_in_tmb branch May 18, 2023 15:47
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.

Add get_cov_chols(<tbd>, type, theta) R function to expose C++ cov Cholesky factor
2 participants