gadget-framework / gadget3

TMB-based gadget implemtation
GNU General Public License v2.0
8 stars 6 forks source link

parscale updated for auto_exp parameters #137

Closed willbutler42 closed 8 months ago

willbutler42 commented 8 months ago

Parscale for auto-exponentiated parameters should be log(upper) - log(lower) (I think) ... have added a line to re-calculate parscale for these instances.

lentinj commented 8 months ago

You're right, but the logic here is getting twisted up. How about something like:

g3_init_val <- function (
        . . . .
        parscale = if (is.null(lower) || is.null(upper)) NULL else 'auto',
        . . . .
        auto_exponentiate = TRUE) {
    stopifnot(identical(parscale, 'auto') || is.numeric(parscale) || is.null(parscale))

 . . .

        if (identical(parscale, 'auto')) {
            # NB: Happens post-auto_exp, so don't need to worry about it
            param_template[matches, 'parscale'] <- diff(c(param_template[matches, 'lower'],
                                                          param_template[matches, 'upper']))
        } else if (!is.null(parscale)) {
          param_template[matches, 'parscale'] <- parscale
          if (any(auto_exp)) param_template[auto_exp, 'parscale'] <- log(param_template[auto_exp, 'parscale'])
        }

Having a magical 'auto' value is a bit annoying, but means we only calculate the default once, and always use post-auto_exp values.

lentinj commented 8 months ago

I think in an ideal world exponentiate would become a column in the parameter template, which would lessen the juggling here. But that's not as quick to solve.

willbutler42 commented 8 months ago

Looks good, will modify accordingly. Also, just realised that we may run into problems if time_ or age_varying = TRUE and exponentiate = TRUE: g3_parameterized('M', by_time = TRUE, exponentiate = TRUE) g3_init_val('*.M.#', 0.15)

This is a bit more fiddly, but something like:

name_re <- paste0(vapply(strsplit(name_spec, ".", fixed = TRUE)[[1]], function (part, auto_exponentiate) {
...
part_exp <- character(1)
if (grepl('[a-zA-Z]', part) && auto_exponentiate) part_exp <- '(_exp)?'
...
return(paste0('(?:\\Q', part, '\\E)', part_exp))
}, character(1)), auto_exponentiate, collapse = "\\.")

name_re <- paste0('^', name_re, '$')

seems to work?

lentinj commented 8 months ago

Also, just realised that we may run into problems if time_ or age_varying = TRUE and exponentiate = TRUE:

I think this is a bug in g3_to_tmb. For g3_parameterized('par.years', value = 0, by_year = TRUE, exponentiate = TRUE) our parameters should be par.years.1980_exp, not par.years_exp.1980. I think this is fixable, though it makes me think that an exponentiate column is an even better idea.

lentinj commented 8 months ago

Try the init_val_exp branch for size, you get:

willbutler42 commented 8 months ago

Try the init_val_exp branch for size, you get:

* [04db866](https://github.com/gadget-framework/gadget3/commit/04db86695cd54191bb87b7b3fe374477e98eb31f) - `_exp` at the end of parameter tables

* [672f1ff](https://github.com/gadget-framework/gadget3/commit/672f1ff012a4250e3c9cb64ad8d1d070ec7505a0) - parscale = "auto", as I was suggesting above.

Thanks. I've just tested this out and everything is working as expected now : )

lentinj commented 8 months ago

Merging https://github.com/gadget-framework/gadget3/pull/138 instead