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

380: Idea to solve convergence problem #386

Merged
merged 13 commits into from
Dec 14, 2023

Conversation

danielinteractive
Copy link
Collaborator

part of #380

@danielinteractive
Copy link
Collaborator Author

@yonicd maybe you are interested in reviewing this design PR - I think I solved the puzzle why SAS converges much better in the example

Copy link
Contributor

github-actions bot commented Dec 13, 2023

badge

Code Coverage Summary

Filename                    Stmts    Miss  Cover    Missing
------------------------  -------  ------  -------  -------------------------------
R/between-within.R             59       0  100.00%
R/component.R                  67       0  100.00%
R/cov_struct.R                 97       1  98.97%   407
R/empirical.R                   7       0  100.00%
R/fit.R                       223      11  95.07%   186-189, 407, 450-453, 468, 498
R/interop-emmeans.R            39       0  100.00%
R/interop-parsnip.R            59       1  98.31%   12
R/kenwardroger.R               92       2  97.83%   41, 63
R/mmrm-methods.R              120       0  100.00%
R/residual.R                    8       8  0.00%    10-31
R/satterthwaite.R             116      12  89.66%   238-249
R/testing.R                    55       4  92.73%   29, 31, 70, 72
R/tidiers.R                    72       2  97.22%   46-47
R/tmb-methods.R               287       3  98.95%   277-278, 338
R/tmb.R                       287       0  100.00%
R/utils-formula.R              27       0  100.00%
R/utils-nse.R                  16       0  100.00%
R/utils.R                     124      10  91.94%   276-286
R/zzz.R                        65      19  70.77%   7-17, 50-55, 85, 113, 133
src/chol_cache.h               63       0  100.00%
src/covariance.h              101       1  99.01%   177
src/derivatives.h             126       0  100.00%
src/empirical.cpp              72       0  100.00%
src/exports.cpp                47       0  100.00%
src/jacobian.cpp               47       1  97.87%   54
src/kr_comp.cpp                56       0  100.00%
src/mmrm.cpp                   76       0  100.00%
src/predict.cpp                93       0  100.00%
src/test-chol_cache.cpp        58       5  91.38%   9, 18, 26, 55, 62
src/test-covariance.cpp       123       5  95.93%   9, 29, 40, 61, 72
src/test-derivatives.cpp      108       7  93.52%   44, 53, 62, 85, 94, 106, 124
src/test-utils.cpp            195       7  96.41%   9, 16, 24, 34, 44, 57, 119
src/testthat-helpers.h         15       5  66.67%   36-37, 41, 50, 53
src/utils.h                    84       0  100.00%
TOTAL                        3084     104  96.63%

Diff against main

Filename      Stmts    Miss  Cover
----------  -------  ------  --------
TOTAL             0       0  +100.00%

Results for commit: da369a6

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

github-actions bot commented Dec 14, 2023

Unit Tests Summary

       1 files       43 suites   23s ⏱️
   462 tests    425 ✔️ 37 💤 0
1 819 runs  1 777 ✔️ 42 💤 0

Results for commit da369a6.

♻️ This comment has been updated with latest results.

@yonicd yonicd self-requested a review December 14, 2023 13:23
- expose latex from comment in chunk to be rendered as part of doc
Copy link
Collaborator

@yonicd yonicd left a comment

Choose a reason for hiding this comment

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

In general i think that the computation cost of the empirical covmat is small and would relax the constraint that this simulated data showed.

To the question of MIVQUE0, i think that could be a longer term goal in the next release. It would give more flexibility for larger dimension designs (per reference paper) so we may hit against that problem anyways as more people use the package.

Copy link
Collaborator

@yonicd yonicd left a comment

Choose a reason for hiding this comment

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

this blogpost may give a direction on a consistent way to resolve NA in the covmat

here is an R package that has similar solutions

@yonicd yonicd self-requested a review December 14, 2023 18:33
@danielinteractive
Copy link
Collaborator Author

this blogpost may give a direction on a consistent way to resolve NA in the covmat

here is an R package that has similar solutions

Thanks Yoni - yeah I think we don't need those more complex solutions because we can start from the residuals data set

Force the PD using Matrix package

```{r}
sigma_emp3_pd <- Matrix::nearPD(sigma_emp3)$mat
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yonicd ah this is cool! thanks

@danielinteractive danielinteractive merged commit 018d0d6 into main Dec 14, 2023
@danielinteractive danielinteractive deleted the 380_idea_unstructured_opt branch December 14, 2023 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants