Genentech / jmpost

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

use cmdstanr generic #397

Closed gowerc closed 2 months ago

gowerc commented 3 months ago

Closes #387

gowerc commented 3 months ago

@gravesti - This is awkward, in order to import the generic from cmdstanr I have to promote cmdstanr to be an imported package rather than suggests, but if I do so it will prevent us from ever uploading to CRAN.... Stuck between a bit of a rock and a hardplace here 😢

github-actions[bot] commented 3 months ago

badge

Code Coverage Summary

Filename                           Stmts    Miss  Cover    Missing
-------------------------------  -------  ------  -------  --------------------------------
R/brier_score.R                      167       0  100.00%
R/DataJoint.R                         76       2  97.37%   264, 270
R/DataLongitudinal.R                 119       1  99.16%   244
R/DataSubject.R                       85       1  98.82%   142
R/DataSurvival.R                      98       0  100.00%
R/defaults.R                          10       6  40.00%   18-57, 84
R/generics.R                          33       4  87.88%   52, 382, 402, 457
R/Grid.R                              27       1  96.30%   173
R/GridEven.R                          32       0  100.00%
R/GridEvent.R                         22       0  100.00%
R/GridFixed.R                         30       0  100.00%
R/GridGrouped.R                       52       0  100.00%
R/GridManual.R                        23       3  86.96%   78-80
R/GridObserved.R                      20       0  100.00%
R/GridPopulation.R                    31       4  87.10%   61, 69-71
R/GridPrediction.R                    36       6  83.33%   83, 91-95
R/JointModel.R                       129      10  92.25%   148-153, 203, 207, 249, 295, 301
R/JointModelSamples.R                 66       0  100.00%
R/link_generics.R                     16       4  75.00%   59, 76, 91, 106
R/Link.R                              62       4  93.55%   200-203
R/LinkComponent.R                     19       3  84.21%   88, 120-121
R/LongitudinalClaretBruno.R           90       6  93.33%   168-172, 187
R/LongitudinalGSF.R                   95       0  100.00%
R/LongitudinalModel.R                 19       0  100.00%
R/LongitudinalQuantities.R            71       0  100.00%
R/LongitudinalRandomSlope.R           44       5  88.64%   102-106
R/LongitudinalSteinFojo.R             86       5  94.19%   159-163
R/Parameter.R                         14       0  100.00%
R/ParameterList.R                     42       1  97.62%   184
R/Prior.R                            254       6  97.64%   507, 651-665
R/Promise.R                           25       0  100.00%
R/Quantities.R                        60       0  100.00%
R/QuantityGeneratorPopulation.R       22       0  100.00%
R/QuantityGeneratorPrediction.R       48       0  100.00%
R/QuantityGeneratorSubject.R          19       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/SimLongitudinalClaretBruno.R        74       0  100.00%
R/SimLongitudinalGSF.R                80       0  100.00%
R/SimLongitudinalRandomSlope.R        42       0  100.00%
R/SimLongitudinalSteinFojo.R          69       0  100.00%
R/SimSurvival.R                      116       0  100.00%
R/StanModel.R                         15       0  100.00%
R/StanModule.R                       176       6  96.59%   192-193, 235, 246, 383, 411
R/SurvivalExponential.R               10       0  100.00%
R/SurvivalGamma.R                     13       0  100.00%
R/SurvivalLoglogistic.R               11       0  100.00%
R/SurvivalModel.R                     19       0  100.00%
R/SurvivalQuantities.R               170       1  99.41%   125
R/SurvivalWeibullPH.R                 13       0  100.00%
R/utilities.R                        144       2  98.61%   13, 335
R/zzz.R                               29      26  10.34%   4-8, 10-31, 38-47
TOTAL                               3117     122  96.09%

Diff against main

Filename        Stmts    Miss  Cover
------------  -------  ------  -------
R/generics.R       -1       0  -0.36%
R/zzz.R            +1      +1  -0.37%
TOTAL               0      +1  -0.03%

Results for commit: 4efa4c5abec546b6801778de8d4efefc18890831

Minimum allowed coverage is 80%

:recycle: This comment has been updated with latest results

gravesti commented 3 months ago

Wonder if something like this could work ? https://forum.posit.co/t/write-s3-methods-for-generics-from-other-packages-without-importing-that-package/8884

gowerc commented 3 months ago

Hmm I don't think that will work as the issue here is the fact that we are trying to assign the method to a generic via the NAMESPACE file not by regular code. Though reading around I wanted to see if this side steps the issue:

As from R 3.6.0 one can also use S3method() directives to perform delayed registration. With

if(getRversion() >= "3.6.0") { S3method(pkg::gen, cls) } function gen.cls will get registered as an S3 method for class cls and generic gen from package pkg only when the namespace of pkg is loaded. This can be employed to deal with situations where the method is not “immediately” needed, and having to pre-load the namespace of pkg (and all its strong dependencies) in order to perform immediate registration is considered too onerous.

https://cran.r-project.org/doc/manuals/R-exts.html#Registering-S3-methods

gowerc commented 3 months ago

@gravesti - So the above appears to work but then causes another issue in that if the user doesn't explicitly load cmdstanr they get the following error when trying to use our package themselves:

Error: could not find function "as.CmdStanMCMC"

To avoid this you would have to have some startup code that automatically loads cmdstanr when our package is loaded but that should really be in the "depends" rather than "suggests" and I doubt CRAN would accept a submission with such a block of code.

Part of me is thinking that we just accept this isn't going to be a CRAN package unless cmdstanr goes to CRAN.

github-actions[bot] commented 2 months ago

Unit Tests Summary

    1 files    178 suites   5m 59s :stopwatch:   156 tests   144 :white_check_mark: 12 :zzz: 0 :x: 1 091 runs  1 078 :white_check_mark: 13 :zzz: 0 :x:

Results for commit 4efa4c5a.

github-actions[bot] commented 2 months ago

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
compile 💚 $6.75$ $-6.23$ $0$ $0$ $0$ $0$
model_multi_chain 💚 $3.91$ $-1.39$ $0$ $0$ $0$ $0$
Additional test case details | Test Suite | $Status$ | Time on `main` | $±Time$ | Test Case | |:-----|:----:|:----:|:----:|:-----| | LongitudinalRandomSlope | 💔 | $20.62$ | $+1.37$ | Print_method_for_LongitudinalRandomSlope_works_as_expected | | SurvivalQuantities | 💔 | $0.90$ | $+5.08$ | SurvivalQuantities_and_autoplot.SurvivalQuantities_works_as_expected | | brierScore | 💚 | $35.62$ | $-1.51$ | brierScore_SurvivalQuantities_returns_same_results_as_survreg | | compile | 💚 | $6.75$ | $-6.23$ | compileStanModel_doesn_t_error_if_the_directory_doesn_t_exist | | model_multi_chain | 💚 | $3.91$ | $-1.39$ | Can_recover_known_distribution_parameters_from_random_slope_model_when_using_multiple_chains |

Results for commit bf17e4792f154ff29d776e24bca7dc5f99b6030b

♻️ This comment has been updated with latest results.