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

sd_phase is turned on during MCMC #109

Open
colemonnahan opened this issue May 22, 2018 · 8 comments
Open

sd_phase is turned on during MCMC #109

colemonnahan opened this issue May 22, 2018 · 8 comments

Comments

@colemonnahan
Copy link
Contributor

I'm not sure this is a bug but it is unexpected behavior that can have some serious consequences wrt run time.

If you look at this line in the manual:

if (sd_phase)
you can see that the sd_phase() function is used to test whether ADMB is operating in the SD phase, and this can be used to simplify calculations during optimization and reduce run time. Which is great.

However, sd_phase() is TRUE during MCMC. You can test it like this:

  if(mc_phase())
   {
      cout << "sd_phase=" << sd_phase() << endl;
   }

If you have something that takes a long time to run wrapped up in a sd_phase clause, it will run for hundreds, thousands or millions of times depending on the length of the MCMC chain. That's bad, as it adds a lot of run time. I just had a model where MCMC runtime is 50 times faster b/c of this. The solution is easy: add a test for MCMC as well.

if(sd_phase() & !mc_phase())
{
  // code that takes a long time to run
}

Now it will be ignored during MCMC but still do the SE calculations at the end of optimization.

I find this behavior very unexpected and suggest that at least it be documented in the manual. I'm not sure whether people use this in their models or if we could simply set the sd_phase() to 0 when entering the MCMC phase. I don't see any use for it and it will only make things take longer to run.

@jimianelli
Copy link
Member

jimianelli commented May 23, 2018 via email

@colemonnahan
Copy link
Contributor Author

I'm inclined to turn that off for my two versions of MCMC. I already broke that histogram calculating code anyway.

@jimianelli
Copy link
Member

jimianelli commented May 23, 2018 via email

@johnoel
Copy link
Contributor

johnoel commented Jan 30, 2021

Hi @Cole-Monnahan-NOAA, it makes sense to set sd_phase to false after the sd_routine(), then the sd_phase will be false in the mcmc phase. This change should not break existing TPL code, but feedback will be needed from others. Please give the issue109 branch a try. It seems to works with existing testing and could make it into the next release admb-12.3.

@johnoel johnoel modified the milestone: ADMB-12.3 Jan 30, 2021
@johnoel
Copy link
Contributor

johnoel commented Feb 2, 2021

@Cole-Monnahan-NOAA has suggested to do further testing to avoid breaking other users code. This will be delayed till the next release after admb-12.3.

@Cole-Monnahan-NOAA
Copy link
Contributor

Coming back to this, perhaps a safer thing is to set it to 0 inside the MCMC functions? That would apply much more narrowly and I think would be a safer approach.

@johnoel johnoel added this to the ADMB-13.1 milestone Aug 9, 2022
@johnoel johnoel removed this from the ADMB-13.1 milestone Dec 20, 2022
@johnoel johnoel closed this as completed Oct 5, 2023
@Cole-Monnahan-NOAA
Copy link
Contributor

@johnoel would you please link to the commits that resolve this for posterity? thanks!

@johnoel
Copy link
Contributor

johnoel commented Oct 5, 2023

This branch was not merged.

@johnoel johnoel reopened this Oct 5, 2023
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

4 participants