refactor recruitment deviations to log space

[Andrea-Havron-NOAA]

What is the feature?

• change recruitment deviations from parameter to log-space

How have you implemented the solution?

• renamed recruit_deviations to log_recruit_devs
• modified functions to account for the devs being in log-space

Does the PR impact any other area of the project?

How to test this change

• test-fims-estimation.R
• test-recruitment-interface.R

Developer pre-PR checklist

• I relied on GitHub actions to 🧪 things for me while I sat on the 🛋️.
• Ran cmake build and ctest locally and the C++ tests passed -->

• Ran devtools::check() locally and the package compiled and all R tests passed. If there are failing tests, run devtools::test(filter = "file_name") (where "test-file_name.R" is the file containing the failing tests) to troubleshoot tests. -->

[Andrea-Havron-NOAA]

changing recruitment deviations to logspace resulted in failing tests in test-fims-estimation. I have compared output from tests to output run from test-fims-estimation on main. All estimated values and negative log-likelihood values are identiical. I suspect tests are failing because gradients are larger with the refactor.

[Andrea-Havron-NOAA]

fixed gradient tests by removing the first log_recruit_dev from calculations as this value was not informing likelihoods. Additionally moved initial logr0 off true value.

The log_recruit_dev vector is now of length nyears-1. If this is how we want to constrain this vector moving forward, I will fix the remaining R and C++ tests

[msupernaw]

@Andrea Havron - NOAA Federal ***@***.***> This is confusing, if log_recruit_dev[0] is not informing the likelihood, why would changing the indexing impact the gradient? It results in an unevaluated branch in the derivative chain (w+=w*dx => w+=w*0 = w).

…

On Mon, Nov 6, 2023 at 3:28 PM Andrea-Havron-NOAA ***@***.***> wrote: fixed gradient tests by removing the first log_recruit_dev from calculations as this value was not informing likelihoods. Additionally moved initial logr0 off true value. The log_recruit_dev vector is now of length nyears-1. If this is how we want to constrain this vector moving forward, I will fix the remaining R and C++ tests — Reply to this email directly, view it on GitHub <#514 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFUSEG4HS4GYKMS2YXPHHLYDFCAHAVCNFSM6AAAAAA65CLBIGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJWGM4DEMZUHA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

-- Matthew Supernaw *Scientific Software Developer* *National Oceanic and Atmospheric Administration* *Office Of Science and Technology* *NOAA Fisheries | *U.S. Department of Commerce Phone 248 - 396 - 7797

[Andrea-Havron-NOAA]

@msupernaw, what I said above wasn't exactly correct. log_recruit_dev[0] is being evaluated in the recruitment likelihood, but nothing from population is informing the parameter. log_recruit_dev[0] is being pulled towards 0 as that is the mean of the rec_nll. I think this issue is related to the fact that there isn't a constraint on the log_rec_dev vector. We will be investigating the best way to constrain the log_recruit_devs in M2 (issue #364) and can look into this issue further then. Typically, this vector should be a length of nyears-1, to account for the lose of a degree of freedom in the constraint.

[Andrea-Havron-NOAA]

We decided to add a new index i_dev so it is a bit more clear that this is different from year-1. We also added documentation throughout explaining that the log_recruit_dev did not include the initial term.

[Andrea-Havron-NOAA]

I have merged main into this branch so do not update using rebase

[codecov]

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (e5c4588) 74.58% compared to head (ceb75b3) 76.10%.

❗ Current head ceb75b3 differs from pull request most recent head 7eb8f86. Consider uploading reports for the commit 7eb8f86 to get more accurate results

Files	Patch %	Lines
...dynamics/recruitment/functors/recruitment_base.hpp	33.33%	6 Missing ⚠️
...e/interface/rcpp/rcpp_objects/rcpp_recruitment.hpp	90.90%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #514      +/-   ##
==========================================
+ Coverage   74.58%   76.10%   +1.52%     
==========================================
  Files          38       37       -1     
  Lines        2034     1791     -243     
  Branches      136        0     -136     
==========================================
- Hits         1517     1363     -154     
+ Misses        476      428      -48     
+ Partials       41        0      -41     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ChristineStawitz-NOAA]

Thanks for this substantial change - at some point during M2 we should revisit the +1 and -1 throughout the indexing since I'm concerned these could lead to user index errors.