Skip to content

Faster configure mcmc#910

Merged
perrydv merged 7 commits into
develfrom
faster-configureMCMC
May 31, 2019
Merged

Faster configure mcmc#910
perrydv merged 7 commits into
develfrom
faster-configureMCMC

Conversation

@perrydv
Copy link
Copy Markdown
Contributor

@perrydv perrydv commented May 29, 2019

This PR provides a toggled processing flow in configureMCMC that calculates traits of nodes by the declaration (resulting in many fewer lookups) and in a more direct manner.

The option useNewConfigureMCMC toggles the new system.

For testing, it is set to TRUE. We could set it to FALSE and include in 0.8.0 or hold off due to congestion going into 0.8.0.

On the "allModels" set of classic BUGS examples in test-models, I see about 10-60% improvements, when useConjugacy = FALSE.

There are more steps that could speed up configureMCMC.

Some of the code in this PR is less readable/maintainable because it uses internal objects more directly. We may want to re-write some of those parts.

@perrydv
Copy link
Copy Markdown
Contributor Author

perrydv commented May 30, 2019

This passed testing with the useNewConfigureMCMC option set to TRUE. We plan to include this in the release but leave it FALSE (off) by default because it was put together hastily and may call for more testing with more creatively weird models. As of this comment I've set it to FALSE and will await testing to confirm that previous behavior is intact.

@perrydv perrydv merged commit 7ffcf67 into devel May 31, 2019
@perrydv perrydv deleted the faster-configureMCMC branch June 3, 2019 15:35
@perrydv perrydv restored the faster-configureMCMC branch August 7, 2019 16:05
@perrydv perrydv deleted the faster-configureMCMC branch October 17, 2019 21:03
@perrydv
Copy link
Copy Markdown
Contributor Author

perrydv commented Dec 9, 2019

@paciorek @danielturek I note that this change in 0.8 provides a switch for faster configureMCMC behavior, which was left off (FALSE) by default. The idea was that we could all be turning it on to turn up any problems, but I'm not sure we've been doing that. Should we run a round of testing and switch it on for 0.9? Or should we make a more diligent effort to test it for a future release?

@perrydv
Copy link
Copy Markdown
Contributor Author

perrydv commented Dec 9, 2019

@paciorek @danielturek On the topic of configureMCMC efficiency, note the following: Checking conjugacy is a slow step. We effectively check conjugacy for all nodes twice because we check it once to assign a conjugate sampler and then we check it again in addSampler if type == "conjugate". The latter check makes sense in a case that a user is adding a conjugate sampler manually and we want to verify its validity, but in a typical processing flow it doubles time spent on conjugacy checking. Could we add an argument to bypass it and use that argument when called from a typical internal processing flow?

@danielturek
Copy link
Copy Markdown
Member

@perrydv Brief comments on your two points:

  1. Yes I'm up for enabling the option useNewConfigureMCMC, and seeing what happens. Will you make that change?

  2. I may have looked too quickly and be wrong here. But I'm not sure that the conjugacy checking happens twice, as you suggest. The initialize method of MCMCconf reference class checks conjugacy for all nodes, as you said. But then, in this initialize method, for conjugate nodes, it calls directly the function addConjugateSampler (emphasis: not the function addSampler), the former of which, addConjugateSampler, doesn't re-check conjugacy. Is that right? Did I over-look something here?

(To be clear: this double-checking would only happen if a user tries to add a conjugate sampler directly themselves, using addSampler, which no one ever does, because it requires a non-trivial control list)

@paciorek
Copy link
Copy Markdown
Contributor

Yes, I've been forgetting to turn that on. I think for future reference if we want to be testing something like that, we should turn it on in devel.

I think you were pretty confident about it, so I think I am ok with including it in release after turning on and testing.

@danielturek
Copy link
Copy Markdown
Member

Ok with me, to enable it if it passes testing, too.

Also need to see if @perrydv agrees with me or not about the "double" checking of conjugacy, which we should fix if he's right.

@perrydv
Copy link
Copy Markdown
Contributor Author

perrydv commented Dec 10, 2019

I will launch a PR to test turning this on.

@danielturek It looked to me like addConjugateSampler calls addSampler, in its last line.

@danielturek
Copy link
Copy Markdown
Member

Ok, I think there are instances here of both of us looking too quickly.

This is my current understanding - which now, is again concluding, that everything is ok - that is, conjugacy check only happens once:

  1. Call to configureMCMC, goes into initialize method.
  2. Conjugacy checking happens once for all nodes (line 218 of MCMC_configure.R)
  3. For conjugate nodes, gets sent to addConjugateSampler (line 303)
  4. Here's the key This function looks up, or generates, the conjSamplerFunction object, which is a non-specialized sampler function. (sampler function generator). It passes this conjSamplerFuntion function object, as the type argument, to addSampler (line 407).
  5. The behaviour of addSampler is "smart", or something. A user can try adding a type = "conjugate" sampler, and it will try to add one, by doing the conjugacy checking. But when the type argument arrives as a function, processing goes to line 461, and no conjugacy checking takes place.

@perrydv Please let me know what you think.

@perrydv
Copy link
Copy Markdown
Contributor Author

perrydv commented Dec 14, 2019

@danielturek Thanks for digging in and explaining that. Makes sense.

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

Successfully merging this pull request may close these issues.

3 participants