Genentech / jmpost

https://genentech.github.io/jmpost/
17 stars 4 forks source link

Support Negative Time / Re-work Simulation function #282

Closed gowerc closed 8 months ago

gowerc commented 8 months ago

Closes #215

This is still a WIP

This ended up being more complicated than expected. Well its just that the current simulation setup could support negative times it was just very clunky and non-intuitive so I took this as an opportunity to overhaul the simulation functions and bring them to parity with the regular JointModel functions (e.g. the api is a lot closer now with key terms being the same with simply a Sim prefix).

Am still updating the test files at the moment as unfortunately the simulation functions are used in a lot of places. Will provide a more broader explanation once I've finished coding.

gowerc commented 8 months ago

@danielinteractive - This is now ready for review. Apologies this ended up being much larger than expected, I appreciate this is more than what should be in a single PR so let me know if you'd prefer to book some time to go over it together.

Main thing is that there are some small updates to the Stan code to ensure both the GSF and SF models work with negative times and then a major overhaul of the simulation functions to bring their syntax inline with what is used for the core model specification.

github-actions[bot] commented 8 months ago

Unit Tests Summary

  1 files   40 suites   7m 50s :stopwatch: 120 tests  89 :white_check_mark: 31 :zzz: 0 :x: 849 runs  818 :white_check_mark: 31 :zzz: 0 :x:

Results for commit 8e761c3b.

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

github-actions[bot] commented 8 months ago

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
JointModelSamples 💔 $33.50$ $+13.04$ $0$ $0$ $0$ $0$
LongitudinalQuantiles 💔 $43.99$ $+17.40$ $0$ $0$ $0$ $0$
LongitudinalRandomSlope 💔 $25.78$ $+21.36$ $+2$ $0$ $0$ $0$
SimGroup 👶 $+0.09$ $+7$ $0$ $0$ $0$
SimJointData 👶 $+1.46$ $+10$ $+1$ $0$ $0$
SimLongitudinalGSF 👶 $+0.13$ $+17$ $+1$ $0$ $0$
SimLongitudinalRandomSlope 👶 $+0.31$ $+17$ $+1$ $0$ $0$
SimLongitudinalSteinFojo 👶 $+0.12$ $+16$ $+1$ $0$ $0$
SimSurvival 👶 $+4.51$ $+5$ $+1$ $0$ $0$
SurvivalExponential 💔 $27.97$ $+13.63$ $-1$ $0$ $0$ $0$
SurvivalLoglogistic 💔 $31.74$ $+11.16$ $0$ $0$ $0$ $0$
SurvivalQuantities 💔 $11.71$ $+7.32$ $+7$ $0$ $0$ $0$
SurvivalWeibullPH 💔 $0.12$ $+41.35$ $+1$ $0$ $0$ $0$
brierScore 💚 $28.40$ $-24.51$ $0$ $0$ $0$ $0$
compile 💔 $14.87$ $+5.20$ $0$ $0$ $0$ $0$
model_multi_chain 👶 $+54.96$ $+1$ $0$ $0$ $0$
model_random_slope 💀 $4.72$ $-4.72$ $-3$ $0$ $0$ $0$
model_random_slope_2chain 💀 $38.16$ $-38.16$ $-1$ $0$ $0$ $0$
simulations 💀 $1.69$ $-1.69$ $-39$ $0$ $0$ $0$
stan_functions 💔 $48.98$ $+17.90$ $0$ $0$ $0$ $0$
Additional test case details | Test Suite | $Status$ | Time on `main` | $±Time$ | Test Case | |:-----|:----:|:----:|:----:|:-----| | JointModelSamples | 👶 | | $+46.53$ | print_works_as_expected_for_JointModelSamples | | JointModelSamples | 💀 | $33.50$ | $-33.50$ | smoke_test_for_JointModelSamples | | LongitudinalQuantiles | 💔 | $2.75$ | $+3.32$ | LongitudinalQuantities_can_recover_known_results | | LongitudinalQuantiles | 💔 | $38.12$ | $+12.79$ | Test_that_LongitudinalQuantities_works_as_expected | | LongitudinalQuantiles | 💔 | $2.23$ | $+1.00$ | autoplot.LongitudinalQuantities_works_as_expected | | LongitudinalRandomSlope | 💔 | $25.71$ | $+12.51$ | LongitudinalRandomSlope_correctly_generates_an_intercept_per_study | | LongitudinalRandomSlope | 👶 | | $+8.81$ | Random_Slope_Model_can_recover_known_parameter_values | | SimGroup | 👶 | | $+0.09$ | SimGroup_works_as_expected | | SimJointData | 👶 | | $+0.83$ | SimJointData_leads_to_valid_DataJoint_with_almost_only_default_arguments | | SimJointData | 👶 | | $+0.37$ | SimJointData_works_as_expected | | SimJointData | 👶 | | $+0.26$ | print_methods_work_as_expected | | SimLongitudinalGSF | 👶 | | $+0.11$ | SimLongitudinalGSF_works_as_expected | | SimLongitudinalGSF | 👶 | | $+0.02$ | print_methods_work_as_expected | | SimLongitudinalRandomSlope | 👶 | | $+0.21$ | SimLongitudinalRandomSlope_correctly_generates_a_dataset_with_known_parameters | | SimLongitudinalRandomSlope | 👶 | | $+0.08$ | SimLongitudinalRandomSlope_works_as_expected | | SimLongitudinalRandomSlope | 👶 | | $+0.02$ | print_methods_work_as_expected | | SimLongitudinalSteinFojo | 👶 | | $+0.10$ | SimLongitudinalSteinFojo_works_as_expected | | SimLongitudinalSteinFojo | 👶 | | $+0.02$ | print_methods_work_as_expected | | SimSurvival | 👶 | | $+1.30$ | SimSurvivalExponential_creates_a_dataset_with_the_correct_parameter | | SimSurvival | 👶 | | $+3.19$ | SimSurvivalWeibullPH_creates_a_dataset_with_the_correct_parameter | | SimSurvival | 👶 | | $+0.02$ | print_methods_work_as_expected | | SurvivalExponential | 💔 | $3.46$ | $+38.02$ | SurvivalExponential_can_recover_true_parameter_including_covariates_ | | SurvivalExponential | 💀 | $24.41$ | $-24.41$ | SurvivalExponential_can_recover_true_parameter_no_covariates_ | | SurvivalLoglogistic | 💔 | $31.56$ | $+11.09$ | SurvivalLogLogistic_can_recover_known_values | | SurvivalQuantities | 💔 | $7.65$ | $+3.57$ | SurvivalQuantities_and_autoplot.SurvivalQuantities_works_as_expected | | SurvivalQuantities | 👶 | | $+2.20$ | SurvivalQuantities_works_with_time_0 | | SurvivalQuantities | 💔 | $3.08$ | $+1.13$ | autoplot.SurvivalSamples_works_as_expected | | SurvivalWeibullPH | 👶 | | $+41.33$ | SurvivalWeibullPH_can_recover_known_values | | brierScore | 💚 | $28.27$ | $-24.60$ | brierScore_SurvivalQuantities_returns_same_results_as_survreg | | compile | 💔 | $14.87$ | $+5.20$ | compileStanModel_doesn_t_error_if_the_directory_doesn_t_exist | | model_multi_chain | 👶 | | $+54.96$ | Can_recover_known_distribution_parameters_from_random_slope_model_when_using_multiple_chains | | model_random_slope | 💀 | $4.72$ | $-4.72$ | Random_Slope_Model_can_recover_known_parameter_values | | model_random_slope_2chain | 💀 | $38.16$ | $-38.16$ | Can_recover_known_distribution_parameters_from_random_slope_model_when_using_multiple_chains | | simulations | 💀 | $0.03$ | $-0.03$ | SimGroup_works_as_expected | | simulations | 💀 | $0.06$ | $-0.06$ | sim_lm_gsf_works_as_expected | | simulations | 💀 | $0.04$ | $-0.04$ | sim_lm_random_slope_works_as_expected | | simulations | 💀 | $0.26$ | $-0.26$ | sim_os_exponential_creates_a_dataset_with_the_correct_parameter | | simulations | 💀 | $0.51$ | $-0.51$ | sim_os_weibull_creates_a_dataset_with_the_correct_parameter | | simulations | 💀 | $0.28$ | $-0.28$ | simulate_joint_data_correctly_generates_an_intercept_per_study | | simulations | 💀 | $0.19$ | $-0.19$ | simulate_joint_data_leads_to_valid_DataJoint_with_almost_only_default_arguments | | simulations | 💀 | $0.32$ | $-0.32$ | simulate_joint_data_works_as_expected | | stan_functions | 💔 | $17.34$ | $+5.57$ | GSF_Identity_Link_Function_works_as_expected | | stan_functions | 💔 | $15.53$ | $+6.16$ | GSF_SLD_function_works_as_expected | | stan_functions | 💔 | $16.10$ | $+6.16$ | Normal_Log_Density_functions_work_as_expected |

Results for commit a8d864f80d9cc4b53807d718b6e717a1daf440fb

♻️ This comment has been updated with latest results.

github-actions[bot] commented 8 months ago

badge

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  --------------------------------
R/brier_score.R                     166       0  100.00%
R/DataJoint.R                        76       2  97.37%   264, 270
R/DataLongitudinal.R                119       1  99.16%   245
R/DataSubject.R                      69       1  98.55%   124
R/DataSurvival.R                     77       0  100.00%
R/defaults.R                         10       6  40.00%   18-57, 84
R/generics.R                         22       1  95.45%   49
R/JointModel.R                      122       8  93.44%   142-144, 194, 198, 240, 286, 292
R/JointModelSamples.R                54       0  100.00%
R/Link.R                             55       4  92.73%   159-162
R/LinkComponent.R                    47       5  89.36%   100, 118, 132-149
R/LongitudinalGSF.R                  64       0  100.00%
R/LongitudinalModel.R                35      12  65.71%   68-83
R/LongitudinalQuantities.R           85       8  90.59%   100-107
R/LongitudinalRandomSlope.R          27       0  100.00%
R/LongitudinalSteinFojo.R            57       8  85.96%   113-135
R/Parameter.R                        14       0  100.00%
R/ParameterList.R                    42       1  97.62%   184
R/Prior.R                           236       8  96.61%   480, 576, 588-606
R/Quantities.R                      105       0  100.00%
R/settings.R                         12      12  0.00%    55-69
R/SimGroup.R                          5       0  100.00%
R/SimJointData.R                     72       1  98.61%   103
R/SimLongitudinal.R                   5       2  60.00%   22, 40
R/SimLongitudinalGSF.R               51       0  100.00%
R/SimLongitudinalRandomSlope.R       42       0  100.00%
R/SimLongitudinalSteinFojo.R         48       0  100.00%
R/SimSurvival.R                     104       0  100.00%
R/StanModel.R                        15       0  100.00%
R/StanModule.R                      177       6  96.61%   199-200, 242, 253, 388, 416
R/SurvivalExponential.R              10       0  100.00%
R/SurvivalLoglogistic.R              11       0  100.00%
R/SurvivalModel.R                    19       0  100.00%
R/SurvivalQuantities.R              155       6  96.13%   178-183
R/SurvivalWeibullPH.R                11       0  100.00%
R/utilities.R                       145       1  99.31%   13
R/zzz.R                               2       2  0.00%    3-12
TOTAL                              2366      95  95.98%

Diff against main

Filename                          Stmts    Miss  Cover
------------------------------  -------  ------  --------
R/generics.R                         +3       0  +0.72%
R/SimGroup.R                         +5       0  +100.00%
R/SimJointData.R                    +72      +1  +98.61%
R/SimLongitudinal.R                  +5      +2  +60.00%
R/SimLongitudinalGSF.R              +51       0  +100.00%
R/SimLongitudinalRandomSlope.R      +42       0  +100.00%
R/SimLongitudinalSteinFojo.R        +48       0  +100.00%
R/SimSurvival.R                    +104       0  +100.00%
R/SurvivalQuantities.R               +4       0  +0.10%
TOTAL                              +334      +3  +1.81%

Results for commit: 8e761c3b095838919718f793d2cea0e3fd39f352

Minimum allowed coverage is 80%

:recycle: This comment has been updated with latest results

gowerc commented 8 months ago

@danielinteractive - Tbh I'm not that worried about the simulation code, at this point it is very well tested and is more auxiliary anyway. If you could focus your attention just on the Stan code changes I think that is more the critical part.

In particular just double checking that indeed the changes mean that 0 and negative times are correctly supported. Talking to Francois today he mentioned about generating quantities for negative times which is something I haven't yet tested so will add some more tests for that.