make unconditional prediction default

[clarkliming]

close #461

for simulate we can open another issue

[danielinteractive]

we should change the approach to "X * beta" for unconditional, because then marginal effects package will also work better with duplicate subject index and visit index

[clarkliming]

to achieve that, we first need to call model.matrix. however it seems that there is a subset https://github.com/openpharma/mmrm/blob/17e417b8b783d2bf8b74fbe3f50650471b4f332b/R/tmb-methods.R#L346 in model.matrix. Do you know why there is @danielinteractive

[danielinteractive]

Good question, seems like from the comment that we want to be sure to only include the utilized observations. What happens if you delete the subset? Does it fail any tests?

[clarkliming]

Good question, seems like from the comment that we want to be sure to only include the utilized observations. What happens if you delete the subset? Does it fail any tests?

of course it fails some tests related to model.matrix, but as you know there are not really much downstream part of model.matrix so hard to say what is the impact.

I can imagine that we should follow

1. if no data provided, always return the utilized obs in that model
2. if new data provided, we should always return based on new data; if NA is in response we should ignore it?
3. however we have the choices to select different "include" option; whether we want to make them consistent (in rows of observations) given different "include"?
4. do we really need the "include" option for model.matrix? (do we really want other dummy columns added to the model.matrix, like subject var, visit_var and group_var? why they should be?)

[danielinteractive]

@ddsjoberg Do you have any opinions on this, because I vaguely remember you added this method? 😃

[ddsjoberg]

@ddsjoberg Do you have any opinions on this, because I vaguely remember you added this method? 😃

I also vaguely recall adding this method 😜 I think when I added this, I had in mind its use with the modeling object. It may be the case that predictions/new data are not being handled they way they are meant to be (I am no expert in the expected behaviour of model.matrix() in various cases).

If there is an update needed, can we ensure the update doesn't break the integration with {broom.helpers}, which does tasks like identifying reference rows and more.

mmrm::mmrm(
    formula = FEV1 ~ SEX + ARMCD + us(AVISIT | USUBJID),
    data = mmrm::fev_data
) |> 
    broom.helpers::tidy_plus_plus()

Should we simply aim to mimic the behaviour of model.matrix.glmmTMB() ?

[ddsjoberg]

If you're not in a rush, I can investigate in the next one to two weeks and make the update to model.matrix()

[danielinteractive]

Thanks a lot @ddsjoberg , that is good to know. We are kind of in a rush because we want to ship another CRAN release soon, and this would be great to include in there to unblock the downstream package marginaleffects. @clarkliming maybe we could add the broom.helpers example as additional test under https://github.com/openpharma/mmrm/blob/main/tests/testthat/test-tmb-methods.R#L571 (adding the corresponding package in Suggests), and then try to refactor?

[github-actions[bot]]

Code Coverage Summary

Filename                    Stmts    Miss  Cover    Missing
------------------------  -------  ------  -------  ----------------------------
R/between-within.R             59       0  100.00%
R/component.R                  69       0  100.00%
R/cov_struct.R                 97       1  98.97%   407
R/empirical.R                   7       0  100.00%
R/fit.R                       229       3  98.69%   420, 481, 511
R/interop-car.R               130       1  99.23%   9
R/interop-emmeans.R            51       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              140       0  100.00%
R/residual.R                    8       0  100.00%
R/satterthwaite.R             116      12  89.66%   238-249
R/skipping.R                    8       0  100.00%
R/testing.R                    64       4  93.75%   29, 31, 80, 82
R/tidiers.R                    72       2  97.22%   46-47
R/tmb-methods.R               294       3  98.98%   162, 306-307
R/tmb.R                       301       1  99.67%   206
R/utils-formula.R              27       0  100.00%
R/utils-nse.R                  16       0  100.00%
R/utils.R                     212      12  94.34%   279-289, 449, 478
R/zzz.R                        72      26  63.89%   7-26, 59-64, 94, 122, 142
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                    78       0  100.00%
TOTAL                        3381      99  97.07%

Diff against main

Filename           Stmts    Miss  Cover
---------------  -------  ------  -------
R/tmb-methods.R      +11       0  +0.04%
R/tmb.R               +5      +1  -0.33%
R/utils.R             +5       0  +0.14%
TOTAL                +21      +1  -0.01%

Results for commit: 2133eb5cc8462ca4d4c66b790fdca4720da259b9

Minimum allowed coverage is 80%

:recycle: This comment has been updated with latest results

[github-actions[bot]]

Unit Test Performance Difference

Test Suite	$Status$	Time on main	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
tmb-methods	💔	$5.83$	$+1.20$	$+5$	$+3$	$0$	$0$

Additional test case details

| Test Suite | $Status$ | Time on `main` | $±Time$ | Test Case | |:-----|:----:|:----:|:----:|:-----| | tmb-methods | 💀 | $0.01$ | $-0.01$ | model.matrix_returns_full_model_frame_if_requested | | tmb-methods | 💀 | $0.01$ | $-0.01$ | model.matrix_works_as_expected_with_includes | | tmb-methods | 👶 | | $+0.93$ | model.matrix_works_with_broom.helpers | | tmb-methods | 👶 | | $+0.03$ | model.matrix_works_with_use_response | | tmb-methods | 👶 | | $+0.01$ | predict_works_for_data_different_factor_levels | | tmb-methods | 👶 | | $+0.01$ | predict_works_for_data_with_duplicated_id | | tmb-methods | 👶 | | $+0.01$ | predict_works_for_unconditional_prediction_if_response_does_not_exist | | utils | 👶 | | $+0.02$ | h_get_na_action_works_for_functions | | utils | 👶 | | $+0.03$ | h_get_na_action_works_for_strings |

Results for commit a67628129c0f192f9c040e54031e60afe9a4ef05

♻️ This comment has been updated with latest results.

[github-actions[bot]]

Unit Tests Summary

    1 files     46 suites   27s :stopwatch:   511 tests   471 :white_check_mark: 40 :zzz: 0 :x: 1 971 runs  1 922 :white_check_mark: 49 :zzz: 0 :x:

Results for commit 2133eb5c.

:recycle: This comment has been updated with latest results.

[danielinteractive]

ok this seems ok apart from the spurious url check problem