More parameterized defaults, replacing g3_param_table

[lentinj]

@willbutler42 here's a bunch of stuff that was keeping me entertained Friday that was supposed to be simple :)

The original aim was to fill in gaps in the defaults, with merging the below you can make a semi-sane substock just by plugging the actions together. The only parameter to set is maxlengthgroupgrowth.

What do you make of the defaults set in https://github.com/gadget-framework/gadget3/commit/953a110984176fa8f05326b6230d48545c373ff6, https://github.com/gadget-framework/gadget3/commit/0feb926f4b24dfa5ec9fe14c37c5f2c2842a07a1, https://github.com/gadget-framework/gadget3/commit/029711865dddae342ca1e0d9ba54988e3a5bb5ac and https://github.com/gadget-framework/gadget3/commit/a30384152e9604950d4a3569559a1a1502d7a9b4? Seem useful?

https://github.com/gadget-framework/gadget3/commit/e565171d6322c813a0be204f1fb79564cf0e3338 and https://github.com/gadget-framework/gadget3/commit/cb5ed2e97b9e84b91c3d96ef4f2dc5406a0a942b are where the fun is. stock_param() & stock_param_table() get removed, as they were fairly short-lived and I'm pretty sure they were never used outside g3_parameterized() (yes?). It's tempting to remove g3_param_table() too, but that is definitely used in existing models so removing it isn't a good plan. Presumably everything recent has moved over to g3_parameterized() by now though?

I might have a bash at turning the g3_pt() nonsense into a proper variable definition before merging this. I think the only awkward bit left is g3_parameterized()'s output turns from plain code into a formlua, which might not be expected everywhere it's used.

[willbutler42]

@willbutler42 here's a bunch of stuff that was keeping me entertained Friday that was supposed to be simple :)

The original aim was to fill in gaps in the defaults, with merging the below you can make a semi-sane substock just by plugging the actions together. The only parameter to set is maxlengthgroupgrowth.

Great : )

What do you make of the defaults set in 953a110, 0feb926, 0297118 and a303841? Seem useful?

These look sensible. I would be tempted to add a cur_step == 1 condition to the run_f default in 0297118, this would be a more common setup than having renewal at each step, so:

run_f = quote( age == stock__minage && !cur_year_projection && cur_step == 1 ),

Also perhaps we don't even need the by_age parameter in g3a_renewal_normalparam?

e565171 and cb5ed2e are where the fun is. stock_param() & stock_param_table() get removed, as they were fairly short-lived and I'm pretty sure they were never used outside g3_parameterized() (yes?). It's tempting to remove g3_param_table() too, but that is definitely used in existing models so removing it isn't a good plan. Presumably everything recent has moved over to g3_parameterized() by now though?

Yes, think its safe to remove stock_param() & stock_param_table(). g3_param_table is definitely used in some older models, so I would keep that for now but with an eye to remove it in the future.

I might have a bash at turning the g3_pt() nonsense into a proper variable definition before merging this. I think the only awkward bit left is g3_parameterized()'s output turns from plain code into a formlua, which might not be expected everywhere it's used.

[lentinj]

These look sensible. I would be tempted to add a cur_step == 1 condition to the run_f default in 0297118, this would be a more common setup than having renewal at each step, so:

run_f = quote( age == stock__minage && !cur_year_projection && cur_step == 1 )

Agreed, generally stuffing everything into the one argument is feeling really clunky now. Having to specify !cur_year_projection because you want cur_step == 2 isn't great.

I've been considering doing something like:-

    run_step = 1,
    run_projection = FALSE,
    run_age = quote( stock__minage ),

...which then &&'s appropriate conditions together into the default run_f.

Also perhaps we don't even need the by_age parameter in g3a_renewal_normalparam?

Yeah, now you say it, it is kinda useless. I think I must have muddled by_age & by_year in my head briefly. Ah well :)

The refactor is probably still worth it though, by_year not working in a variable will bite at some point.

[willbutler42]

These look sensible. I would be tempted to add a cur_step == 1 condition to the run_f default in 0297118, this would be a more common setup than having renewal at each step, so: run_f = quote( age == stock__minage && !cur_year_projection && cur_step == 1 )

Agreed, generally stuffing everything into the one argument is feeling really clunky now. Having to specify !cur_year_projection because you want cur_step == 2 isn't great.

I've been considering doing something like:-

    run_step = 1,
    run_projection = FALSE,
    run_age = quote( stock__minage ),

...which then &&'s appropriate conditions together into the default run_f.

Yes, I like this idea :)

Also perhaps we don't even need the by_age parameter in g3a_renewal_normalparam?

Yeah, now you say it, it is kinda useless. I think I must have muddled by_age & by_year in my head briefly. Ah well :)

The refactor is probably still worth it though, by_year not working in a variable will bite at some point.

Whilst we are on the topic of defaults, perhaps a good opportunity to remove the scaling for default parameters? This was carried over from the g2 setup scripts, but now that we are using optim(control = list(parscale = TRUE)), it is probably not necessary & will likely just cause confusion for users expecting K to be K rather than 0.001 * K. Think these are the instances (excluding documentation):

https://github.com/gadget-framework/gadget3/blob/master/R/action_grow.R#L4 https://github.com/gadget-framework/gadget3/blob/80fc249aca78c83bba513d0eb5c2ac379b49f922/R/action_renewal.R#L10 https://github.com/gadget-framework/gadget3/blob/80fc249aca78c83bba513d0eb5c2ac379b49f922/R/action_mature.R#L2

[lentinj]

I had wondered about 0.001 * K. I'm certainly game to remove it, if we don't do it now it's only going to get harder.

The question will be how manageable the fallout is, i.e. how many models will be expecting the old default. Even older models that use their own default would be fine.

[willbutler42]

I had wondered about 0.001 * K. I'm certainly game to remove it, if we don't do it now it's only going to get harder.

The question will be how manageable the fallout is, i.e. how many models will be expecting the old default. Even older models that use their own default would be fine.

The fallout will be minimal. None of the benchmarked models use these particular defaults, K and mat_alpha are all spelled out in gadgetutils, so think it is pretty safe to remove, and as you say, a good thing to do now.

[lentinj]

Likewise, I've done mat.alpha too`

[bthe]

Looks good, one thing that I can think of is that it might be worth reworking g3a_renewal_vonB so it would

a) use a more commonly used parametrisation of Linf, K and t_0 (instead of recl) b) adjust of the time of the initial conditions, as the mean length should be estimated at age - delta_t

[lentinj]

This pull request got wildly out of hand. Closing this in place of #118 & #61 with much tidier associated branches - since we don't strictly need it here I've broken out the g3_param_table stuff which can get merged once the temporary g3_pt is dealt with.