[Bug]: recruitment nll

[Andrea-Havron-NOAA]

Describe the bug

The recruitment nll still evaluates when estimate_log_devs set to false

To Reproduce

In the fims demo vignette, estimate_log_devs is set to false. The model run should return recruitment nll of 0, but it instead returns the nll evaluated at the fixed rec_devs, using report$rec_nll.

Screenshots

Which OS are you seeing the problem on?

Windows, Linux

Which browser are you seeing the problem on?

No response

Which version of FIMS are you seeing the problem on?

commit 9f13e8d

Additional Context

No response

[iantaylor-NOAA]

@Andrea-Havron-NOAA, thanks for raising this issue.

My thoughts differ on the solution, however. I think we want the ability to fix recdevs at non-zero values. And if the nll is identically zero when they get fixed then I think it will mess up our ability to do likelihood profiles. If you want zero nll contribution from recdevs, wouldn't it be sufficient to do something like

recruitment$estimate_log_devs <- FALSE
recruitment$log_devs <- rep(0, n_years)

[Andrea-Havron-NOAA]

@iantaylor-NOAA, thanks for your response. In this sense, does the recruitment nll act as a penalty on the model? Regardless of whether the rec_devs are estimated, the user still wants to evaluate the nll of the rec_devs?

[Andrea-Havron-NOAA]

The reason for raising this issue is that the current implementation of FIMS has a conditional statement that returns an nll=0 if rec_devs are not being estimated (see here). This conditional statement is not working though because the bool isn't being correctly passed into the model.

[Andrea-Havron-NOAA]

@Andrea-Havron-NOAA, thanks for raising this issue.

My thoughts differ on the solution, however. I think we want the ability to fix recdevs at non-zero values. And if the nll is identically zero when they get fixed then I think it will mess up our ability to do likelihood profiles. If you want zero nll contribution from recdevs, wouldn't it be sufficient to do something like

recruitment$estimate_log_devs <- FALSE
recruitment$log_devs <- rep(0, n_years)

This would still return an nll contribution because log(dnorm(0,0,sigma_r)) has a non-zero value unless sigma_r = 1/(sqrt(2*pi)). I tend to prefer flags to control whether or not an nll contributes to the overall jnll.

[iantaylor-NOAA]

@Andrea-Havron-NOAA, good points.

First, I appreciate that it would be confusing if there is a nll component associated with recdevs in an age-structured production model where you have no intention of estimated recruitment. It's good to have the full likelihood even that means the nll isn't 0 when the fit is perfect (e.g. recdevs = 0) as noted by @Cole-Monnahan-NOAA in nmfs-ost/ss3-source-code#553.

In addition to likelihood profile case (which would require being able to fix a single recdevs so maybe not worth worrying about yet) a more relevant example in the short term is the case studies comparing FIMS to existing assessments. One potential step in the comparison is fixing all parameters, including recdevs, at the estimates from the non-FIMS model and comparing the likelihood to the case where those parameters are estimated within FIMS. If the likelihood for the recdevs goes away when they're fixed, it's harder to understand how much better the fit is when they're estimated.

One potential solution would be to include a weight which is applied to each likelihood component and set that weight to 0 for recdev nll if you are not interested in them at all. The R output design doc: https://docs.google.com/document/d/1n-3D_ywHhmPNITMU6Jjsh7D1ixYc_CcvckKfdPWBGDM/edit?pli=1#heading=h.ghe64oz7zkw includes a "weight" column in the "fits" output that is intended the report any likelihood weighting applied to each data point. It probably makes sense to have a similar weight available for the likelihood components that aren't associated with data, such as recdevs.

[iantaylor-NOAA]

Sorry, I should have added that it's fine to just fix the passing of the boolian to the model to get the current configuration working as intended with nll fixed at 0 while thinking about more flexible configurations for the future.

[Andrea-Havron-NOAA]

Thanks @iantaylor-NOAA for your detailed response. I have two aims here. One is to fix the current bug by passing in the boolean and keeping or removing the conditional statement based on user expectations. The other is to better understand how the recrutiment nll component should be handled when rec devs are not estimated as we are about to start planning a refactor that will create a more generic framework for initiating nll components. In my plan, the nll component will only exist if initiated by the user, so there will be no need for conditional statements to turn off nll evaluations. I think it will still be possible to initiate the nll for recruitment as a penalty when rec_devs are not estimated in the model.

[Andrea-Havron-NOAA]

Sorry, to be more clear, I plan do a hot fix to the main which would impact case studies, and a more long term change based on the nll refactor plans. For the hot fix, is it better for the case studies to leave the evaluation of rec_nll as is when log_devs are not estimated?

[Cole-Monnahan-NOAA]

I think we want the nll to be zero when that flag is false. Profiling a recdev is probably a rare case and a weird one too, for reasons outside the scope of this issue. But even when done I think the user will need to use the mapping feature on the R side, and not this estimate flag. I did a profile in my case study here. So the flag would still be to estimate=true b/c the other devs would be estimated anyway I think.

So I see no harm in having them zeroed out when not estimated.