Skip to content
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

Open
johnoel opened this issue Apr 20, 2018 · 13 comments

Comments

@johnoel
Copy link
Contributor

johnoel commented Apr 20, 2018

No description provided.

@iantaylor-NOAA
Copy link

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
Copy link
Contributor

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

@iantaylor-NOAA
Copy link

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
Copy link

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)

if (!initial_params::mc_phase)

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
Copy link
Contributor

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
Copy link

@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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

Cole-Monnahan-NOAA commented Apr 9, 2020 via email

@johnoel
Copy link
Contributor Author

johnoel commented Apr 10, 2020

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

@kellijohnson-NOAA
Copy link

@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
Copy link

Has the discrepancy been corrected?

@johnoel johnoel reopened this Oct 5, 2023
@johnoel
Copy link
Contributor Author

johnoel commented Oct 5, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants