admb-project / admb

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

sd_phase is turned on during MCMC #109

Open colemonnahan opened 6 years ago

colemonnahan commented 6 years ago

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: https://github.com/admb-project/admb/blob/05271017d4d523b0943c1cf556c0f7c2e0594b34/docs/manuals/admb/admb.tex#L1518 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 commented 6 years ago

There is a reason (albeit currently defunct) for this behavior...the original mcmc code could be linked to some java graphics package to update the marginals of sdreport variables (as a density histogram). At the time this was there to demonstrate how the chains were progressing (or not) and was mostly gee whiz.

On Tue, May 22, 2018 at 3:34 PM Cole Monnahan notifications@github.com wrote:

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: https://github.com/admb-project/admb/blob/05271017d4d523b0943c1cf556c0f7c2e0594b34/docs/manuals/admb/admb.tex#L1518 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.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/admb-project/admb/issues/109, or mute the thread https://github.com/notifications/unsubscribe-auth/AClv4mDvYyX1_FRiIm20qPFtg3f5U6gFks5t1JJegaJpZM4UJfw7 .

-- Jim Ianelli

colemonnahan commented 6 years ago

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

jimianelli commented 6 years ago

works for me!

On Wed, May 23, 2018 at 11:13 AM Cole Monnahan notifications@github.com wrote:

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

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/admb-project/admb/issues/109#issuecomment-391446410, or mute the thread https://github.com/notifications/unsubscribe-auth/AClv4neXZx04YzGfZDNfMgtebAAdrlVFks5t1abOgaJpZM4UJfw7 .

-- Jim Ianelli

johnoel commented 3 years ago

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 commented 3 years ago

@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 commented 2 years ago

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.

Cole-Monnahan-NOAA commented 11 months ago

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

johnoel commented 11 months ago

This branch was not merged.