search

metrumresearchgroup / bbr

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

max_eval overwrites estimation methods for complex models #519

Closed barrettk closed 2 years ago

barrettk commented 2 years ago

Previously, test_threads had not been tested with complex models (models with multiple estimation methods). Updating MAXEVAL/NITER, would cause the second estimation method to inherit the first. Additionally, in cases where the first estimation method used MAXEVAL, and the second one used NITER, both would have MAXEVAL (not illustrated)

The below example illustrates the behavior, where the second $EST line became SAEM instead of IMP

Original ctl file:

$EST METHOD=SAEM INT EONLY=0 NBURN=3000 NITER=250  PRINT=5  GRD=DDDDDDDDDDDD CTYPE=3 SEED=2021  RANMETHOD=P
CALPHA=0.05 ISAMPLE=2
$EST METHOD=IMP INT EONLY=1 NITER=4 ISAMPLE=3000 PRINT=1  GRD=DDDDDDDDDDDD RANMETHOD=P  SEED=2021 CTYPE=3  FILE=28h.EXT
CALPHA=0.05 MSFO=28h.MSF
$COV UNCONDITIONAL MATRIX=R PRINT=E

The ctl file generated by the test_threads function:

$EST METHOD=SAEM INT EONLY=0 NBURN=3000  PRINT=5  GRD=DDDDDDDDDDDD CTYPE=3 SEED=2021  RANMETHOD=P NITER=10
CALPHA=0.05 ISAMPLE=2
$EST METHOD=SAEM INT EONLY=0 NBURN=3000  PRINT=5  GRD=DDDDDDDDDDDD CTYPE=3 SEED=2021  RANMETHOD=P NITER=10
CALPHA=0.05 MSFO=28h.MSF
$COV UNCONDITIONAL MATRIX=R PRINT=E
barrettk commented 2 years ago

Not sure why drone was failing here in the previous commit. typed.missing = TRUE is a valid argument, and prevents users from passing NA and stuff like that (assert_numeric will allow that by default). Its not necessary (cant imagine a user actually passing that value), so I just removed it to appease drone.

image

barrettk commented 2 years ago

@seth127 I requested Kyle to review this PR, but tagging you in case you'd like to take a look/be involved

kyleam commented 2 years ago

Not sure why drone was failing here in the previous commit.

The mpn:oldest build was failing. That uses the 2020-06-08 snapshot, which has checkmate 2.0.0:

$ curl -fSsL https://mpn.metworx.com/snapshots/stable/2020-06-08/src/contrib/PACKAGES | grep -A1 'Package: checkmate'
Package: checkmate
Version: 2.0.0

The typed.missing argument wasn't added to checkmate until 434a2cf (Flag to control handling of missing values, 2020-10-14), which was a part of the v2.1.0 release.

So, dropping the argument (as you've already done) is the way to go, unless we bump the minimum MPN that we test against.

seth127 commented 2 years ago

Thanks @barrettk . From a quick glance, this all looks good to me, but I'll let @kyleam do a real review.