James-Thorson-NOAA / FishStatsUtils

Shared resources for spatio-temporal models
GNU General Public License v3.0
10 stars 16 forks source link

suggestion: fit_model checks for mid-level objects (e.g. extrapolation_list) #83

Closed N-DucharmeBarth-NOAA closed 2 years ago

N-DucharmeBarth-NOAA commented 2 years ago

Hi Jim, As a suggestion for the next round of updates, it could be useful for fit_model() to check for the presence of objects (e.g. extrapolation_list, spatial_list, data_list, or tmb_list) created by the mid-level functions . If one of these items is detected in the arguments list then the corresponding code in fit_model creating that object could be skipped. For example if extrapolation_list is in the arguments of fit_model() then this section of code could be skipped using the following:

if(missing(extrapolation_list))
{
 # Build extrapolation grid
  message("\n### Making extrapolation-grid")
  extrapolation_args_default = list(Region = settings$Region,
                             strata.limits = settings$strata.limits,
                             zone = settings$zone,
                             max_cells = settings$max_cells,
                             DirPath = working_dir)
  extrapolation_args_input = combine_lists( input = extra_args,
                           default = extrapolation_args_default,
                           args_to_use = formalArgs(make_extrapolation_info) )
  extrapolation_list = do.call( what=make_extrapolation_info, args=extrapolation_args_input )
}

The mid-level objects would also have to be added to the formal arguments of the function but without a default value in order to use missing(). This would greatly speed up the execution of fit_model() if users have already created/modified some of the mid-level objects and are passing them into the function as extra arguments. Best, Nicholas

James-Thorson-NOAA commented 2 years ago

That's a great idea :)

If I implement this on the dev branch would that be sufficient for your purposes? Or perhaps you want to make a local copy of whatever release you're using, and make the change locally there to avoid having to update to the dev branch?

N-DucharmeBarth-NOAA commented 2 years ago

Either is fine with me so whatever is easiest for you at this stage. Do you want me to submit a pull request to the dev branch with the suggested changes to fit_model() implemented?

James-Thorson-NOAA commented 2 years ago

Picking this after some thought, I could do this two ways:

  1. I could do as you suggest, and list extrapolation_list and spatial_list as arguments to fit_model and if they're missing build them;
  2. I could search for them in extra_args which is where ... goes (i.e., the user can pass them as unnamed arguments), which would avoid cluttering the args(fit_model) and fit_model documentation.

can you explain what use-cases would require a user to define their own extrapolation_list or spatial_list objects, so I can guess how often this would come up?

Jim

N-DucharmeBarth-NOAA commented 2 years ago

Option 2 seems to be the cleaner option to me.

A use case for extrapolation_list would be to modify the a_el object to define strata for a user defined region. However, it is possible that I just don't know how to do this effectively with existing functionality 😄. For spatial_list this could potentially be to modify the penalty on spatial decorrelation when using the "Barrier" method.

Admittedly, these use cases are rather specific. However I think they would fall under the umbrella of custom modifications for advanced users described on the VAST wiki. Adding a check for the mid-level functions would presumably also help other users making custom modifications for particular cases.

James-Thorson-NOAA commented 2 years ago

OK makes sense! See latest commit to dev branch. I'm closing for now, but please confirm if this works for you or throws some unexpected error.

N-DucharmeBarth-NOAA commented 2 years ago

Awesome, thanks! This appears to work fine for my specific application. fit_model() is much faster now that it skips the creation of extrapolation_list and spatial_list.