admb-project / admb

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

param_init_bounded_dev_vector has incorrect values when used with -mcmc option. #107

Open johnoel opened 6 years ago

iantaylor-NOAA commented 6 years ago

Here's some additional information on this issue.

In nh99/model5.cpp there is a penalty applied to the sum of squared values and then the sum is subtracted. However, in mc_phase neither of these things happens, allowing the dev_vector to have sum different from 0. Perhaps the penalty was supposed to apply in the mc_phase and the subtracting elsewhere or vice versa.

The attached version of pella_t has extra cout to report likelihood, first dev_vector value, and sum of dev_vector during mcmc and mceval (filenames have .txt added to file to allow attaching to github). This shows that shows that sum(effort_devs) are never very close to 0 during mcmc but very close to 0 during mceval. Other than MLE value, parameter values and likelihoods reported during mceval never appear during mcmc, suggesting that mcmc algorithm is not sampling the parameter space correctly for any model that uses an init_bounded_dev_vector:

MCMC: 202 obj_fun: -217.491 effort_devs(1): -0.0505607 sum(effort_devs): -0.0203968
...
MCeval: 202 obj_fun: -217.455 effort_devs(1): -0.0501964 sum(effort_devs): 3.29597e-016

pella_t_modified.tpl.txt pella_t_modified.dat.txt

colemonnahan commented 6 years ago

@johnoel is there any progress on understanding the implications of changing that line that Ian referenced?

iantaylor-NOAA commented 6 years ago

For the record, further discussion of this issue is on the ADMB Developers email list here: https://groups.google.com/a/admb-project.org/forum/#!topic/developers/8QNAd3a_iGQ

iantaylor-NOAA commented 5 years ago

Update: following a suggestion from @johnoel, I tried compiling ADMB after commenting out line 23 from admb/src/nh99/model5.cpp: if (!initial_params::mc_phase) https://github.com/admb-project/admb/blob/1f46f242d502a6cab6f518dfc6b9f0782b90931d/src/nh99/model5.cpp#L23 This seemed to remove the discrepancy between the parameter values reported in the .psv file and the parameters available during the mceval_phase without causing any other problems for the mcmc sampling.

Further testing should be done to make sure this fix indeed works and doesn't create other issues.

colemonnahan commented 5 years ago

I will continue to contend, ad nauseam, that we developers should not make this change until we can fully demonstrate the implications. I suggest someone recreate the vector type (with penalties) in a stand-alone .tpl and then explicitly show that you get the same thing. I will also add that for the hybrid MCMC options (including NUTS) there are likely implications for Jacobians when you do these penalties and I recommend someone explore that during these tests.

kellijohnson-NOAA commented 4 years ago

@colemonnahan do you recall which stock assessment model you were running that lead to 10% of the mcmc runs having crashed populations as you note in the google discussion group?

Cole-Monnahan-NOAA commented 4 years ago

It was a toy cod model from the ss3sim package. But I believe the issue is a problem whenever a stock gets close to crashing. Then, slight changes to recdevs can (will?) land you in crash territory. I noticed the issue when -mceval draws had huge crash penalties.

kellijohnson-NOAA commented 4 years ago

Good to know. I am planning on doing some simulation testing of using simple deviations in SS vs. the bounded_dev_vector.

Cole-Monnahan-NOAA commented 4 years ago

OK sounds good. Let me know if I can be of help.

On Thu, Apr 9, 2020 at 1:59 PM Kelli Johnson notifications@github.com wrote:

Good to know. I am planning on doing some simulation testing of using simple deviations in SS vs. the bounded_dev_vector.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/admb-project/admb/issues/107#issuecomment-611749698, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOBEDUHZ5RZKHWQ3Z7URJ43RLYZMJANCNFSM4E3YWYMQ .

johnoel commented 4 years ago

This issue will be on the agenda for the upcoming workshop. For now, I would recommend not using the _dev types.

kellijohnson-NOAA commented 4 years ago

@johnoel is your recommendation for MLE and MCMC or just the latter? Also, when you say it is on the agenda do you mean that you hope to find a fix for this during the workshop? In the meantime, I will continue with explorations of what this bug means in terms of Stock Synthesis as we must report to managers why and how results change when we stop using the bounded_dev_vector for recruitment deviations.

Rick-Methot-NOAA commented 11 months ago

Has the discrepancy been corrected?

johnoel commented 11 months ago

There is a fix. In the test example, it seems to work. However, the changes may affect other models.

See https://github.com/admb-project/admb/pull/284/files