admb-project / admb

AD Model Builder
http://admb-project.org
Other
64 stars 19 forks source link

suggestion: optimization should honor -maxph #199

Closed wStockhausen closed 2 years ago

wStockhausen commented 3 years ago

This is just a suggestion: optimization should honor the -maxph command line setting. Currently, the setting of -maxph (assuming it's set on the command line) is ignored if it is less than the max parameter phase when minimizing the objective function (see code for function_minimizer::minimize() in xmodelm3.cpp). This is probably not what the user expects when setting -maxph less than the max parameter phase. The reason for setting -maxph less than the max parameter phase (unless accidental) is likely to be that the user wants to examine issues associated with model convergence obtained by turning on parameters at different phases. Currently, if the user wants to stop optimization after phase x (and perhaps calculate the hessian or estimate sd's at this point), he/she must turn off all parameters with phase > x in the tpl code or, if the model supports it, a control file. It would be much more convenient in this use case if ADMB simply honored the -maxph setting when determining the actual max optimization phase. I have a modified version of function_minimizer::minimize() that appears to do this correctly that I'm happy to provide (email me at william.stockhausen@noaa.gov).

Cole-Monnahan-NOAA commented 3 years ago

Is this not the sole purpose of the option? I'm confused if not. This should definitely work. I was able to reproduce this in 12.3 with the catage model where catage -maxfn 1 optimized all the way up to phase 3. It seems to have no effect.

wStockhausen commented 3 years ago

Not quite sure what you mean by "it has no effect". I hope you mean "setting maxph (not maxfn, btw) to less than the max parameter phase has no effect". I'm not sure, but I think the original point of maxph was to allow the user to extend the optimization to phases greater than the max parameter phase. This might be the case if you wanted to change the weighting applied to the data in the likelihood after you had all the parameters turned on--a tuning step of some sort (e.g., McAllister-Ianelli for size comps).

William T. Stockhausen

Research Fishery Biologist, Alaska Fisheries Science Center

NOAA Fisheries | U.S. Department of Commerce

Office: (206) 526-4241

www.fisheries.noaa.gov

On Wed, Mar 24, 2021 at 1:58 PM Cole Monnahan @.***> wrote:

Is this not the sole purpose of the option? I'm confused if not. This should definitely work. I was able to reproduce this in 12.3 with the catage model where catage -maxfn 1 optimized all the way up to phase 3. It seems to have no effect.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/admb-project/admb/issues/199#issuecomment-806181648, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMAD5FLQ7BZ2U4C4WVSCJTTFJG6HANCNFSM4ZVLLBUA .

iantaylor-NOAA commented 3 years ago

I was also confused by the purpose of -maxph, but the ADMB manual makes it clear that @wStockhausen is correct

-maxph N You may want to add extra phases to the minimization—usually because the standard set of phases has not converged. This will set the number of phases to N."

My confusion stems from using Stock Synthesis which has a setting for "Turn off estimation for parameters entering after this phase" which often gets referred to as the "max phase". However, this setting appears to work exactly as @wStockhausen noted in the original post: the TPL code changes the phase for any parameter with phase greater than the turn of phase to be negative.

@johnoel, how hard would it be to make -maxph work for both purposes: either increasing the maximum phase as it works currently, or decreasing to allow explorations of what's happening at intermediate phases?

Cole-Monnahan-NOAA commented 3 years ago

So the idea is if you have a max phase of 5 but the model hasn't converged at the end of that phase you can trick it into running more iterations by doing -maxph 20. It'll essentially restart the optimizer 15 times? Why not just set the maxfn to something larger so it keeps going? I'm baffled by this option. Personally I'd much rather see it work the way @wStockhausen proposed.

maxph N means to stop the optimizer progressing beyond phase N.

wStockhausen commented 3 years ago

Again, it's also useful to allow maxph to be set > the max parameter phase. It's all about flexibility, I think. You may never use it, but it's nice to have the potential to do so. For example, for a model I use that fits size composition data, I want to be able to change the effective sample sizes using tuning algorithms (e.g., McAllister-Ianelli) after arriving at the MLE with the input sample sizes. I could do this by calculating the new effective sample sizes after the end of the last parameter estimation phase (in the FINAL_SECTION) and writing them to a file, then read the file after the program ends, update the input sample sizes in my dat file, and run the model again (possibly repeating this several times until the input and output effective sample sizes converge). However, it's a lot less time consuming (once you develop the code to do it) if you can set maxph > max parameter phase and update the sample sizes in the BETWEEN_PHASES_SECTION after each subsequent phase.

William T. Stockhausen

Research Fishery Biologist, Alaska Fisheries Science Center

NOAA Fisheries | U.S. Department of Commerce

Office: (206) 526-4241

www.fisheries.noaa.gov

On Thu, Mar 25, 2021 at 11:08 AM Cole Monnahan @.***> wrote:

So the idea is if you have a max phase of 5 but the model hasn't converged at the end of that phase you can trick it into running more iterations by doing -maxph 20. It'll essentially restart the optimizer 15 times? Why not just set the maxfn to something larger so it keeps going? I'm baffled by this option. Personally I'd much rather see it work the way @wStockhausen https://github.com/wStockhausen proposed.

maxph N means to stop the optimizer progressing beyond phase N.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/admb-project/admb/issues/199#issuecomment-807206773, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMAD5EOXYWZK4PVSLEBQCDTFN3Z5ANCNFSM4ZVLLBUA .

Cole-Monnahan-NOAA commented 3 years ago

Very interesting example I'm sold that both uses should work ideally. @wStockhausen you said you had code, do you mind pasting it in here and see if @johnoel can try it out? I'm happy to test as well if that's helpful.

wStockhausen commented 3 years ago

In xmodelm3.cpp, paste in the code below over the original starting at "void function_minimizer::minimize(void)" and ending at " if ( (on=option_match(ad_comm::argc,ad_comm::argv,"-ndv",nopt))>-1)" In the following, the bits bracketed by "//WTS--" and "//--WTS" or appended with "//WTS" are my additions (most of it is original code):

void function_minimizer::minimize(void) { int nopt=0; int on=0; if ( (on=option_match(ad_comm::argc,ad_comm::argv,"-shess"))>-1) { laplace_approximation_calculator::sparse_hessian_flag=1; } if ( (on=option_match(ad_comm::argc,ad_comm::argv,"-pis"))>-1) {

laplace_approximation_calculator::print_importance_sampling_weights_flag=1; } if ( (on=option_match(ad_comm::argc,ad_comm::argv,"-sp"))>-1) { laplace_approximation_calculator::saddlepointflag=1; }

if defined(__MINI_MAX__)

    if ( (on=option_match(ad_comm::argc,ad_comm::argv,"-mm"))>-1)
    {
      laplace_approximation_calculator::saddlepointflag=2;
    }

else

    if ( (on=option_match(ad_comm::argc,ad_comm::argv,"-mm"))>-1)
    {
       cerr << "option -mm MINI_MAX not defined " << endl;
       ad_exit(1);
    }

endif

//initial_params::read(); // read in the values for the initial

parameters if (initial_params::restart_phase) { initial_params::current_phase = initial_params::restart_phase; initial_params::restart_phase=0; } int allphases=initial_params::max_number_phases; if ( (on=option_match(ad_comm::argc,ad_comm::argv,"-maxph",nopt))>-1) { //WTS-- int origMPs = initial_params::max_number_phases; int jj = 0; //--WTS if (!nopt) { cerr << "Usage -maxph option needs integer -- ignored" << endl; } else { //WTS-- // int jj=atoi(ad_comm::argv[on+1]); jj=atoi(ad_comm::argv[on+1]); //--WTS if (jj<=0) { cerr << "Usage -maxph option needs positive integer -- ignored.\n"; } else { if (jj>allphases) { allphases=jj; } } } if (allphases>initial_params::max_number_phases) { initial_params::max_number_phases=allphases; } //WTS-- if (jj<allphases){ allphases = jj; initial_params::max_number_phases=allphases; cout<<"NOTE: max_number_of_phases reset from "<<origMPs<<" to " <<initial_params::max_number_phases << endl; } //--WTS } if ( (on=option_match(ad_comm::argc,ad_comm::argv,"-ndv",nopt))>-1)

William T. Stockhausen

Research Fishery Biologist, Alaska Fisheries Science Center

NOAA Fisheries | U.S. Department of Commerce

Office: (206) 526-4241

www.fisheries.noaa.gov

On Thu, Mar 25, 2021 at 11:52 AM Cole Monnahan @.***> wrote:

Very interesting example I'm sold that both uses should work ideally. @wStockhausen https://github.com/wStockhausen you said you had code, do you mind pasting it in here and see if @johnoel https://github.com/johnoel can try it out? I'm happy to test as well if that's helpful.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/admb-project/admb/issues/199#issuecomment-807278259, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMAD5FOM3ZERVIF7EEA4CTTFOA65ANCNFSM4ZVLLBUA .

Cole-Monnahan-NOAA commented 2 years ago

I implemented this fix on this branch: https://github.com/Cole-Monnahan-NOAA/admb/tree/issue199

and tested it locally and it seemed to work. @johnoel could we get this into the next release? Please advise on how you'd like me to do a PR

Thanks @wStockhausen for bringing this up and proposing code. I think it'll be a nice addition to ADMB's functionality.

Cole-Monnahan-NOAA commented 2 years ago

I just tested this out on the new 13.0 branch and I'm not sure it's working as it should. @johnoel can you confirm this got merged in correctly? I'd like to test this and close this issue before release.

johnoel commented 2 years ago

I will check again...

johnoel commented 2 years ago

Merged in Pull #254.