-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
param_init_bounded_dev_vector has incorrect values when used with -mcmc option. #107
Comments
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).
|
@johnoel is there any progress on understanding the implications of changing that line that Ian referenced? |
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 |
Update: following a suggestion from @johnoel, I tried compiling ADMB after commenting out line 23 from admb/src/nh99/model5.cpp: Line 23 in 1f46f24
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. |
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. |
@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? |
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. |
Good to know. I am planning on doing some simulation testing of using simple deviations in SS vs. the bounded_dev_vector. |
OK sounds good. Let me know if I can be of help.
…On Thu, Apr 9, 2020 at 1:59 PM Kelli Johnson ***@***.***> 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
<#107 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AOBEDUHZ5RZKHWQ3Z7URJ43RLYZMJANCNFSM4E3YWYMQ>
.
|
This issue will be on the agenda for the upcoming workshop. For now, I would recommend not using the _dev types. |
@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. |
Has the discrepancy been corrected? |
There is a fix. In the test example, it seems to work. However, the changes may affect other models. |
No description provided.
The text was updated successfully, but these errors were encountered: