Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

g3_parameterized()-based defaults #118

Closed
lentinj opened this issue Aug 9, 2023 · 9 comments · Fixed by #119
Closed

g3_parameterized()-based defaults #118

lentinj opened this issue Aug 9, 2023 · 9 comments · Fixed by #119

Comments

@lentinj
Copy link
Collaborator

lentinj commented Aug 9, 2023

See #117 for more discussion - enough parametrized defaults so we can throw together a ~sensible model by just combining actions.

This was referenced Aug 9, 2023
lentinj added a commit that referenced this issue Aug 10, 2023
Any understocking should be heavily penalised, make a high weight the
default.
lentinj added a commit that referenced this issue Aug 10, 2023
There's only one option, so make it the default.

Don't bubble up the by_stock / by_age options, as using these is
currently a bit hairy at the moment, see:

  #113

If you want a more parameterized M, it's a bit more sensible to do this
yourself, example added to show how to do this.
lentinj added a commit that referenced this issue Aug 10, 2023
The final add_dependent_formula call wasn't substituting stock names,
run g3_step again so it does.

Ideally this would become part of add_dependent_formula.

In the process we have another f_optimize() pass over the code,
resulting in lots of codegeneration changes.
lentinj added a commit that referenced this issue Aug 10, 2023
We only renew at the youngest age, so having a quick by_age option
doesn't make any sense.

It'd be possible to disable "age == stock__minage" in which case it
becomes relevant, but then you're not really describing renewal.
lentinj added a commit that referenced this issue Aug 10, 2023
The TRUE default is never useful, will at least want:
* Only renew at the minimum age
* Not a projection year (in which case there's no data)
And possibly want:
* Step in which to renew at

Break up the default clauses into their own parameters, and combine
them by default.

NB: This is assuming that a stock has an age dimension, which it may
not do. Should we be wrapping in if (stock_has_an_age_dimension)?
lentinj added a commit that referenced this issue Aug 10, 2023
If any of Linf / K / init.sd / rec.sd are zero, then renewal will
result in an NaN stock.

These defaults aren't useful either, but at least a model fresh out of
the box will return something other than NaN.
lentinj added a commit that referenced this issue Aug 10, 2023
It's historical, ripping the band-aid off now will hurt less than it
will in the future.
lentinj added a commit that referenced this issue Aug 10, 2023
It's historical, ripping the band-aid off now will hurt less
than it will in the future.
@lentinj
Copy link
Collaborator Author

lentinj commented Aug 10, 2023

@bthe says

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

@willbutler42 any suggestions on what this would look like? We might end up with 2 vonB functions though, which would be a bit annoying from a naming point of view.

@bthe
Copy link
Collaborator

bthe commented Aug 10, 2023

I guess something like:

g3a_renewal_vonb <- function(
        Linf = g3_parameterized('Linf', by_stock = by_stock),
        K = g3_parameterized('K', by_stock = by_stock, scale = 0.001),
        t0 = g3_parameterized('t0', by_stock = by_stock),
        by_stock = TRUE) {
    f_substitute(
        quote( Linf * (1 - exp(-1 * K * (age - t0))) ),
        list(
            Linf = Linf,
            K = K,
            recl = t0))
}

would take care of my first bullet.

The second bullet could be hacked by adding deltat to t0, but it may be clearer to define an offset parameter to the function.

@lentinj
Copy link
Collaborator Author

lentinj commented Aug 10, 2023

Adding t0 = g3_parameterized('t0', by_stock = by_stock, offset = "deltat") should cover this (I think!).

But I think we'd need separate g3a_renewal_vonb_t0() & g3a_renewal_vonb_recl(). I guess the long-term ideal would be to switch the default g3a_renewal_vonb to being g3a_renewal_vonb_t0, but again that's a chunk of short-term pain.

lentinj added a commit that referenced this issue Aug 10, 2023
Rebase went wrong, and this got a sneak-preview of the #61 g3_pt_lookup()
work.
@bthe
Copy link
Collaborator

bthe commented Aug 10, 2023

Sound reasonable. I guess that you can go from g3a_renewal_vonb_t0 to g3a_renewal_vonb_recl by specifying t0 as (recage + log(1 - recl/Linf)/K)

@lentinj
Copy link
Collaborator Author

lentinj commented Aug 10, 2023

Okay, so I think the thing to do is:

  • Have explicit g3a_renewal_vonb_t0() & g3a_renewal_vonb_recl(). Make g3a_renewal_vonb() an alias for g3a_renewal_vonb_recl().
  • Change the default in g3a_renewal_normalparam() to g3a_renewal_vonb_t0()

So models that used g3a_renewal_vonb() explicitly won't break, which I think is more likely to be the case for older models.
Models that don't specify mean_f OTOH will need updating.

We could eventually tidy up the alias if we felt it was worth it once the dust has settled.

@lentinj
Copy link
Collaborator Author

lentinj commented Aug 10, 2023

Also (sorry!) what do you both say to:

 g3a_renewal_normalparam <- function (
         stock,
         factor_f = g3_parameterized('rec',
             by_stock = by_stock,
             by_year = TRUE,
-            scale = g3_parameterized(name = 'rec.scalar', by_stock = by_stock),
+            scale = g3_parameterized(
+                name = 'rec.scalar',
+                by_stock = by_stock,
+                by_step = is.null(run_step)),
             ifmissing = NaN),
         mean_f = g3a_renewal_vonb(by_stock = by_stock),
         stddev_f = g3_parameterized('rec.sd', value = 10, by_stock = by_stock),
         alpha_f = g3_parameterized('walpha', by_stock = wgt_by_stock),
         beta_f = g3_parameterized('wbeta', by_stock = wgt_by_stock),
         by_stock = TRUE,
         wgt_by_stock = TRUE,
         run_age = quote(stock__minage),
         run_projection = FALSE,
         run_step = 1,
         run_f = NULL,
         run_at = 8) {

...i.e. if you set run_step = NULL, continuous recruitment, then you automatically get per-step rec.scalar parameters.

This would be needed for the anchovy continuous-renewal-apart-from-in-winter, but seems fairly generally applicable.

@bthe
Copy link
Collaborator

bthe commented Aug 10, 2023

Looks fine to me. If one wants have different recruitment by year and step I assume you just do:

factor_f = g3_parameterized('rec',
             by_stock = by_stock,
             by_year = TRUE,
             scale = g3_parameterized(name = 'rec.scalar', by_stock = by_stock),
             by_step = TRUE)

@lentinj
Copy link
Collaborator Author

lentinj commented Aug 10, 2023

Exactly. Or we could make your suggestion the default if you specify continuous recruitment (although it would result in a fair few additional parameters).

Either would be useful I'm sure, I guess the question is which would "do the right thing" in more cases.

lentinj added a commit that referenced this issue Aug 10, 2023
Add what should be a more familiar parameterisation of the vonb
function, and use it by default instead.

Add an explicit g3a_renewal_vonb_recl(), which g3a_renewal_vonb() is an
alias for. This way models that use g3a_renewal_vonb() won't change,
and you can use g3a_renewal_vonb_recl() explicitly if required.
lentinj added a commit that referenced this issue Aug 10, 2023
If renewal is configured to be continuous, then a per-step rec.scalar
should be more useful than a fixed scalar.
@bthe
Copy link
Collaborator

bthe commented Aug 11, 2023

Exactly. Or we could make your suggestion the default if you specify continuous recruitment (although it would result in a fair few additional parameters).

Either would be useful I'm sure, I guess the question is which would "do the right thing" in more cases.

I think the timestep specific scalar is more appropriate, unless you have too much data lying around (I wish). But I think that it would good to have this as an example in the help page for renewal.

lentinj added a commit that referenced this issue Aug 20, 2023
In retrospect, I'm not convinced doing this automagically is the right
way to go, we don't with any other options.

Remove this before it becomes relied upon.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants