get parameter names in fims output

[Andrea-Havron-NOAA]

What is the feature?

• issue [feature:] do we need a get_parameter_names function? [feature:] do we need a get_parameter_names function? #666

How have you implemented the solution?

• add code to fit_fims to return parameter names to TMB objects
• add example to fims_demo

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

• this PR modifies fit_fims and the fims_demo

[github-actions]

Instructions for code reviewer

Hello reviewer, thanks for taking the time to review this PR!

• Please use this checklist during your review, checking off items that you have verified are complete!
• For PRs that don't make changes to code (e.g., changes to README.md or Github actions workflows), feel free to skip over items on the checklist that are not relevant. Remember it is still important to do a thorough review.
• Then, comment on the pull request with your review indicating where you have questions or changes need to be made before merging.
• Remember to review every line of code you’ve been asked to review, look at the context, make sure you’re improving code health, and compliment developers on good things that they do.
• PR reviews are a great way to learn, so feel free to share your tips and tricks. However, for optional changes (i.e., not required for merging), please include nit: (for nitpicking) before making the suggestion. For example, nit: I prefer using a data.frame() instead of a matrix because...
• Engage with the developer when they respond to comments and check off additional boxes as they become complete so the PR can be merged in when all the tasks are fulfilled. Make it clear when this has been reached by commenting on the PR with something like This PR is now ready to be merged, no changes needed.

Checklist

• The PR is requested to be merged into the appropriate branch (typically main)
• The code is well-designed.
• The functionality is good for the users of the code.
• Any User Interface changes are sensible and look good.
• The code isn’t more complex than it needs to be.
• Code coverage remains high, indicating the new code is tested.
• The developer used clear names for everything.
• Comments are clear and useful, and mostly explain why instead of what.
• Code is appropriately documented (doxygen and roxygen).

[Cole-Monnahan-NOAA]

Before I review this, is there a way to assign the new names into obj right after making it, so that everything downstream that TMB produces has the names we want? That would likely be a better and more general solution than updating names here.

[kellijohnson-NOAA]

@Cole-Monnahan-NOAA I worry about altering the obj or opt inside of fit_fims() because individuals that fit models without using the wrapper will have different output.

@Andrea-Havron-NOAA the PR is currently failing on all three operating systems because of an issue with the length of either get_parameter_names(obj$par) or the object being forced onto it. I will investigate. I am also going to alter the PR to assign the vector of names to the names stored in estimates rather than altering obj or opt.

[Cole-Monnahan-NOAA]

@kellijohnson-NOAA I think we should expect that advanced users will still use fit_fims to build the object, just flagging do_fit=FALSE or whatever it's called now. That returns the obj which would have the updated names. Just something to think about as it would probably simplify a lot of the FIMS code. @Andrea-Havron-NOAA what do you think?

I'm not suggesting

[kellijohnson-NOAA]

Thanks @Cole-Monnahan-NOAA for the thoughts. After thinking about this some, I think you are right 👍 . Putting the names in obj will not cause anything to break if those names are not available so we should just do it. @Andrea-Havron-NOAA what do you think?

[Andrea-Havron-NOAA]

@Cole-Monnahan-NOAA I worry about altering the obj or opt inside of fit_fims() because individuals that fit models without using the wrapper will have different output.

@Andrea-Havron-NOAA the PR is currently failing on all three operating systems because of an issue with the length of either get_parameter_names(obj$par) or the object being forced onto it. I will investigate. I am also going to alter the PR to assign the vector of names to the names stored in estimates rather than altering obj or opt.

@kellijohnson-NOAA, it's likely the names vector isn't being cleared properly. Every time a test is run, the vector gets larger. I'll look into this. Follow-up: I just pushed up code that clears the parameter names. Checks are all passing now except for the Mac-OS test that's failing on dev.

[Andrea-Havron-NOAA]

Thanks @Cole-Monnahan-NOAA for the thoughts. After thinking about this some, I think you are right 👍 . Putting the names in obj will not cause anything to break if those names are not available so we should just do it. @Andrea-Havron-NOAA what do you think?

I agree. The code only modifies the names of the obj components. it doesn't add anything new.

[Cole-Monnahan-NOAA]

I haven't tested this idea so we should do that before committing to it. Specifically the names attributes are stored in different locations so we'd want to make sure we get it right.

[msupernaw]

What if we use default names in the Rcpp objects, but allow the user to override them? For instance, given a logistic selectivity module, with id 1, the inflection_point name could be "logistic_selectivity_1.inflection_point".d This code could easily be set in the object constructors and held in a container in information that points to the string name in the Rcpp object. If the name changes, the changes will be reflected in information. I think this could be a good way to extend what's already been developed and provides a reasonable way forward on bookkeeping the parameter names.

…

On Thu, Oct 17, 2024 at 12:23 PM Cole Monnahan ***@***.***> wrote: I haven't tested this idea so we should do that before committing to it. Specifically the names attributes are stored in different locations so we'd want to make sure we get it right. — Reply to this email directly, view it on GitHub <#676 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFUSEAM3TJWIEDQUKPL3DLZ37QBJAVCNFSM6AAAAABPZUKPUCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJZHE4DMOBUG4> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

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

[Andrea-Havron-NOAA]

What if we use default names in the Rcpp objects, but allow the user to override them? For instance, given a logistic selectivity module, with id 1, the inflection_point name could be "logistic_selectivity_1.inflection_point".d This code could easily be set in the object constructors and held in a container in information that points to the string name in the Rcpp object. If the name changes, the changes will be reflected in information. I think this could be a good way to extend what's already been developed and provides a reasonable way forward on bookkeeping the parameter names.
…
On Thu, Oct 17, 2024 at 12:23 PM Cole Monnahan @.> wrote: I haven't tested this idea so we should do that before committing to it. Specifically the names attributes are stored in different locations so we'd want to make sure we get it right. — Reply to this email directly, view it on GitHub <#676 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABFUSEAM3TJWIEDQUKPL3DLZ37QBJAVCNFSM6AAAAABPZUKPUCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJZHE4DMOBUG4 . You are receiving this because you are subscribed to this thread.Message ID: @.>
-- Matthew Supernaw Mathematical Statistician National Oceanic and Atmospheric Administration Office Of Science and Technology *NOAA Fisheries | *U.S. Department of Commerce Phone 248 - 396 - 7797

I think this warrants the discussion of whether or not we want users to override the default names. There is the case that we are hoping to use FIMS to enforce a more standardized naming convention on parameter names. Also with helper functions, users will not need to set names in the Rcpp interface. I am open to arguments for overriding default parameter names though!

[Andrea-Havron-NOAA]

I haven't tested this idea so we should do that before committing to it. Specifically the names attributes are stored in different locations so we'd want to make sure we get it right.

I think better code would acquire the names from the C++ objects instead of setting them manually, similar to what REPORT does in TMB. I just haven't figured out how to implement this yet.

[msupernaw]

*..., similar to what REPORT does in TMB. I just haven't figured out how to implement this yet.* #define STRINGIFY(x) #x

…

On Thu, Oct 17, 2024 at 2:50 PM Andrea-Havron-NOAA ***@***.***> wrote: I haven't tested this idea so we should do that before committing to it. Specifically the names attributes are stored in different locations so we'd want to make sure we get it right. I think better code would acquire the names from the C++ objects instead of setting them manually, similar to what REPORT does in TMB. I just haven't figured out how to implement this yet. — Reply to this email directly, view it on GitHub <#676 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFUSEAHO47CYXEQFAUJOS3Z4ABGPAVCNFSM6AAAAABPZUKPUCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMRQGI4TEOJZGQ> . You are receiving this because you commented.Message ID: ***@***.***>

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

[iantaylor-NOAA]

Happy to have the discussion, but my perspective is that wherever we store the parameter names, I would strongly recommend against making it easy for users overriding them. If users want to use different parameter names in tables or figures, they can apply some translation prior to creating those things. But if it happens too close to the core of FIMS it will surely break our standardized output processing tools.

I think this warrants the discussion of whether or not we want users to override the default names. There is the case that we are hoping to use FIMS to enforce a more standardized naming convention on parameter names. Also with helper functions, users will not need to set names in the Rcpp interface. I am open to arguments for overriding default parameter names though!