metrumresearchgroup / bbr

R interface for model and project management
https://metrumresearchgroup.github.io/bbr/
Other
23 stars 2 forks source link

param_estimates and initial_estimates use different names for THETA records #672

Closed barrettk closed 5 months ago

barrettk commented 6 months ago

As you can see below, THETA names as presented via param_estimates() use THETAX as the identifier, whereas they are presented as THETA(X) via initial_estimates():

> MOD1 %>% model_summary() %>% param_estimates()
# A tibble: 9 × 8                                                                                                                                                                            
  parameter_names estimate   stderr random_effect_sd random_effect_sdse fixed diag  shrinkage
  <chr>              <dbl>    <dbl>            <dbl>              <dbl> <lgl> <lgl>     <dbl>
1 THETA1            2.32   8.72e- 2           NA              NA        FALSE NA        NA   
2 THETA2           54.6    3.38e+ 0           NA              NA        FALSE NA        NA   
3 THETA3          463.     3.03e+ 1           NA              NA        FALSE NA        NA   
4 THETA4           -0.0820 5.61e- 2           NA              NA        FALSE NA        NA   
5 THETA5            4.18   1.38e+ 0           NA              NA        FALSE NA        NA   
6 OMEGA(1,1)        0.0985 2.03e- 2            0.314           3.24e- 2 FALSE TRUE      17.5 
7 OMEGA(2,1)        0      1   e+10            0               1   e+10 TRUE  FALSE     NA   
8 OMEGA(2,2)        0.157  2.72e- 2            0.396           3.44e- 2 FALSE TRUE       2.07
9 SIGMA(1,1)        1      1   e+10            1               1   e+10 TRUE  TRUE       4.08

> MOD1 %>% initial_estimates()
# A tibble: 8 × 6
  parameter_names  init lower_bound upper_bound record_type record_number
  <chr>           <dbl>       <dbl>       <dbl> <chr>               <int>
1 THETA(1)         2              0          NA theta                   1
2 THETA(2)         3              0          NA theta                   1
3 THETA(3)        10              0          NA theta                   1
4 THETA(4)         0.02          NA          NA theta                   1
5 THETA(5)         1             NA          NA theta                   1
6 OMEGA(1,1)       0.05          NA          NA omega                   1
7 OMEGA(2,2)       0.2           NA          NA omega                   1
8 SIGMA(1,1)       1             NA          NA sigma                   1

These should be the same so that users can more easily join these two tables together. Given how much longer param_estimates() has been an available function (and how records are specified in the yaml spec files via pmparams), it makes sense to change initial_estimates() to match the names in param_estimates().

seth127 commented 6 months ago

Thanks for filing this @barrettk

@kyleam do you see any issue with making this change? I discussed offline with @barrettk and I think it's worth it, given the likely use case (of wanting to join these two df's together). And, given that initial_estimates() is so new, I don't have too much worry about this being a breaking change. That said, it is technically a breaking change. Any thoughts or input is welcome.

kyleam commented 6 months ago

@kyleam do you see any issue with making this change?

No, I agree with your assessment.