Fixes the inability to map parameters

[kellijohnson-NOAA]

What is the feature?

• Allows users to map parameters to other parameters with the argument map to TMB::MakeADFun()
• Adds warning messages when the object passed to set_fixed() is not the same length as what is expected because before R would crash.

How have you implemented the solution?

• Uses obj[["env"]]$parList(opt[["par"]])[["p"]] instead of opt[["par"]]

Does the PR impact any other area of the project, maybe another repo?

• No 😁

---

Instructions for code reviewer

👋Hello reviewer👋, thank you for taking the time to review this PR!

• Please use this checklist during your review, checking off items that you have verified are complete but feel free to skip over items that are not relevant!
• See the GitHub documentation for how to comment on a PR to indicate where you have questions or changes are needed before approving the PR.
• Please use standard conventional messages for both commit messages and comments
• PR reviews are a great way to learn so feel free to share your tips and tricks. However, when suggesting changes to the PR that are optional please include nit: (for nitpicking) as the comment type. For example, nit: I prefer using a data.frame() instead of a matrix because ...
• Engage with the developer. Make it clear when the PR is approved by selecting the approved status, and potentially commenting on the PR with something like This PR is now ready to be merged.

Checklist

• The PR requests the appropriate base branch (dev for features and main for hot fixes)
• The code is well-designed
• The code is designed well for both users and developers
• Code coverage remains high- [ ] Comments are clear, useful, and explain why instead of what
• Code is appropriately documented (doxygen and roxygen)

[kellijohnson-NOAA]

According to Google the difference between opt[["par"]] and obj[["env"]]$parList(opt[["par"]])[["p"]]

In TMB (Template Model Builder), the difference between opt[["par"]] and obj[["env"]]$parList(opt[["par"]])[["p"]] lies in whether the parameters are presented as a vector of all parameters (as used by the optimizer) or as a structured list of parameters (mapped back to the structure defined in your C++ code).Here is the breakdown of the comparison:opt[["par"]]: This is the output from an optimizer like nlminb(). It is a simple, named numeric vector containing all estimated parameters (both fixed and random, depending on how MakeADFun was called) concatenated together.obj[["env"]]$parList(opt[["par"]])[["p"]]: This uses the TMB environment function parList to convert that flat numeric vector opt[["par"]] back into a structured list of parameters. The [["p"]] at the end typically selects a specific parameter vector or matrix, for example parList() returns a list containing parameters defined in C++ as PARAMETER(p).

[codecov]

Codecov Report

❌ Patch coverage is 68.75000% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.20%. Comparing base (c57cf29) to head (faf66be).

Files with missing lines	Patch %	Lines
inst/include/interface/rcpp/rcpp_interface.hpp	64.86%	13 Missing ⚠️
R/reshape_output.R	71.42%	6 Missing ⚠️
R/fimsfit.R	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1406      +/-   ##
==========================================
+ Coverage   82.97%   89.20%   +6.23%     
==========================================
  Files          54       83      +29     
  Lines        2214     8590    +6376     
  Branches      579      583       +4     
==========================================
+ Hits         1837     7663    +5826     
- Misses        279      895     +616     
+ Partials       98       32      -66     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Andrea-Havron-NOAA]

It does not look like the code here is set up to handle random effects models.

I would use instead: ‘derived_quantity_nrow <- summary(std, “report”) |> nrow()’

Then:
‘random_nrow <- summary(std, “random”) |> row()’

[Andrea-Havron-NOAA]

I don’t understand the purpose of set_fixed. It looks like it is only used just before sdreport, but that function defines par within the function, so par is being redefined by sdreport. I am also very uncomfortable giving users the ability to overwrite info->fixed_effects_parameters and info->random_effects_parameters. It seems like doing so could cause a lot of unintended consequences in optimization an SE calculation. TMB’s obj manages parameter updates the parameters in obj should point to info->fixed_effects_parameters and info->random_effects_parameters

[Andrea-Havron-NOAA]

When map is invoked, I think we need to fix the mismatch between obj$par and info->fixed_effects_parameters and info->random_effects_parameters immediately after MakeADFun because any run with cause a mismatch in parameters here which will cause errors. Also the model will be trying to optimize parameters that are not linked to anything in the model, which will lead to convergence errors.

I think we should pull out map IDs from obj somehow and use these to update fixed and random effects vectors in info before doing anything else. We will still need to track what has been mapped to link back up with obj$env$parList().

[Andrea-Havron-NOAA]

When map is invoked, I think we need to fix the mismatch between obj$par and info->fixed_effects_parameters and info->random_effects_parameters immediately after MakeADFun because any run with cause a mismatch in parameters here which will cause errors. Also the model will be trying to optimize parameters that are not linked to anything in the model, which will lead to convergence errors.

I think we should pull out map IDs from obj somehow and use these to update fixed and random effects vectors in info before doing anything else. We will still need to track what has been mapped to link back up with obj$env$parList().

I dug into this a bit more and wanted to correct my comment above. When using the map argument, TMB does not modify the length of PARAMETER_VECTOR(p) in FIMS.cpp, so we do not need to modify the length of info->fixed_effects_parameters and info->random_effects_parameters. I still think we should remove set_fixed and set_random, but other than that this PR is correct in trying to fix the output so dimensionality lines up.