g3_param(eterized) conflicting definitions / using M by-age

[lentinj]

In @mmrinconh 's model uses an age-varying M. This appears easy to set up:

g3a_naturalmortality(anch, g3a_naturalmortality_exp(by_age = TRUE))

But that doesn't work, since we also use M in g3a_renewal_initabund(), so also need to tell it there:

  g3a_initialconditions_normalparam(anch,
    factor_f = g3a_renewal_initabund(M = g3_parameterized('M', by_stock = TRUE, by_age = TRUE)),
    by_age = TRUE),

This is all kinda ugly.

• At least we should highlight you need to do this in the documentation.
• We could have a shortcut option to by_age, say M_by_age, but I'm not sold on that being clearer.
• We should be able to detect these conflicts and give a decent explanation when formulating the code, rather than generating nonsense C++ you then have to derive an explanation from.
• Post https://github.com/gadget-framework/gadget3/issues/61 we might be able to do something where g3a_renewal_initabund() assumes M is already defined, but that is tricky since we need the by_stock to know what the M parameter is called.

[willbutler42]

In @mmrinconh 's model uses an age-varying M. This appears easy to set up:

g3a_naturalmortality(anch, g3a_naturalmortality_exp(by_age = TRUE))

But that doesn't work, since we also use M in g3a_renewal_initabund(), so also need to tell it there:

  g3a_initialconditions_normalparam(anch,
    factor_f = g3a_renewal_initabund(M = g3_parameterized('M', by_stock = TRUE, by_age = TRUE)),
    by_age = TRUE),

This is all kinda ugly.

* At least we should highlight you need to do this in the documentation.

Agreed.

* We could have a shortcut option to `by_age`, say `M_by_age`, but I'm not sold on that being clearer.

I am not sold on this either. Perhaps removing the by_age parameter from g3a_naturalmortality_exp function is the best way to avoid this scenario? So the user has to supply the parameter formula if they want the age dimension.

* We should be able to detect these conflicts and give a decent explanation when formulating the code, rather than generating nonsense C++ you then have to derive an explanation from.

Yep. Along those lines, at some stage I would like to gather together some snapshots of C++ nonsense with some suggested fixes, things like muddled parameters, action order issues etc

* Post [Rework parameter inclusion (turn g3_param into "real" function) #61](https://github.com/gadget-framework/gadget3/issues/61) we might be able to do something where `g3a_renewal_initabund`() assumes `M` is already defined, but that is tricky since we need the `by_stock` to know what the `M` parameter is called.

[lentinj]

Perhaps removing the by_age parameter from g3a_naturalmortality_exp function is the best way to avoid this scenario?

I see what you're saying, and it's very tempting. But I'm loath to make a breaking change just because you might have done the wrong thing.

As you say, getting the user to do it is probably the only sensible way, and isn't so bad:

M <- g3_parameterized('M', by_stock = TRUE, by_age = TRUE)
  . . .
  g3a_naturalmortality(anch, g3a_naturalmortality_exp(M))
  g3a_initialconditions_normalparam(anch, factor_f = g3a_renewal_initabund(M = M), by_age = TRUE),

FWIW I don't think #61 is as far off as I initially thought (partially because some of the refactor is necessary for by_age to work properly). Then it will be an explicit error: "M can't be defined as both this and that", which would definitely make this not-so-bad.

[lentinj]

@bthe is suggesting making M by_age the default, which does feel like a more sensible default.

Given this is a breaking change in itself, then removing the by_age parameter as suggested above isn't so much of a problem. If we got around to doing "NA-means-map-to-previous" in the options, then that would provide a simpler way to turn M-by-age off than modifying it's definition anyway.

[lentinj]

Another conflicting definition is g3a_grow_lengthvbsimple()s K & g3a_renewal_vonb_t0()s K.

[bthe]

Could you give me an example? Both should be derived from the standard Von B equation, so the K should be the same.

[lentinj]

We discussed this earlier, but for sake of completeness:-

> st_imm <- g3_stock(c(species = "fish", 'imm'), seq(5L, 25L, 5)) |> g3s_age(1L, 5L)
> st_mat <- g3_stock(c(species = "fish", 'mat'), seq(5L, 25L, 5)) |> g3s_age(3L, 10L)
> stocks = list(imm = st_imm, mat = st_mat)
> actions_st_imm <- list(
+   g3a_growmature(st_imm,
+     g3a_grow_impl_bbinom(
+       by_stock = 'species',
+       maxlengthgroupgrowth = 4L )),
+   g3a_initialconditions_normalcv(st_imm),
+   NULL)
> names(attr(suppressWarnings(g3_to_r(actions_st_imm)), 'parameter_template'))
 [1] "fish_imm.init.scalar" "fish_imm.init.1"      "fish_imm.init.2"
 [4] "fish_imm.init.3"      "fish_imm.init.4"      "fish_imm.init.5"
 [7] "fish_imm.M.1"         "fish_imm.M.2"         "fish_imm.M.3"
[10] "fish_imm.M.4"         "fish_imm.M.5"         "init.F"
[13] "recage"               "fish_imm.Linf"        "fish_imm.K"
[16] "fish_imm.t0"          "fish_imm.lencv"       "fish_imm.walpha"
[19] "fish_imm.wbeta"       "fish.Linf"            "fish.K"
[22] "fish.bbin"            "fish.wbeta"           "fish.walpha"

Note that there's both a fish_imm.K (from initialconditions) and fish.K (from growmature).

[lentinj]

For these cases (VonB params & M), where the values are considered key properties of the stock, and used everywhere, I'm currently thinking the solution is a bit more indirection, something like:

st_imm <- g3_stock(c(species = "fish", 'imm'), seq(5L, 25L, 5)) |> g3s_age(1L, 5L)
st_mat <- g3_stock(c(species = "fish", 'mat'), seq(5L, 25L, 5)) |> g3s_age(3L, 10L)
st_imm$env$stock__M <- g3_parameterized('M', by_stock = 'species', by_age = TRUE)
st_mat$env$stock__M <- g3_parameterized('M', by_stock = 'species', by_age = TRUE)

g3_to_r(list(
    g3a_naturalmortality(st_imm, g3a_naturalmortality_exp( ~stock__M )),
    g3a_naturalmortality(st_mat, g3a_naturalmortality_exp( ~stock__M )),
    NULL))

Obviously the environment-jiggling would be neater in the real thing, maybe as part of g3_stock():

st_imm <- g3_stock(
    c(species = "fish", 'imm'), seq(5L, 25L, 5),
    param_by_stock = 'species') |> g3s_age(1L, 5L)
st_mat <- g3_stock(
    c(species = "fish", 'mat'), seq(5L, 25L, 5),
    param_by_stock = 'species') |> g3s_age(3L, 10L)

g3_to_r(list(
    g3a_naturalmortality(st_imm, g3a_naturalmortality_exp( ~stock__M )),
    g3a_naturalmortality(st_mat, g3a_naturalmortality_exp( ~stock__M )),
    NULL))

It also needs to actually work, currently fish_imm__M has been set outside age loop, not inside. Not sure why.