search

nimble-dev / nimble

The base NIMBLE package for R
http://R-nimble.org
BSD 3-Clause "New" or "Revised" License
160 stars 24 forks source link

error trap multivariate samplers assigned to target nodes of RJMCMC #1471

Closed paciorek closed 4 months ago

paciorek commented 5 months ago

As discussed with a user, it doesn't make sense to have a mv sampler assigned to a node that is being configured for RJMCMC. That sampler will be toggled off and wouldn't sample the other parameters that it is supposed to sample. And we don't have a way to have the mv sampler operate in the reduced dimension space when the target node is not in the model anyway.

So we need to add an error trap that having mv sampler on a target node is not allowed.

While we are thinking about this, we should also consider what happens if the target node has two samplers (either two univariate or one univar and one mv) assigned to it. I don't know that we would handle this correctly - presumably we want to toggle off both samplers. I haven't checked what would actually happen.

danielturek commented 5 months ago

Agreed that these cases need some attention. My initial thought would be to error-trap any of these non-standard cases: when anything other than a single, univariate sampler, is operating on a node specified for RJMCMC.

I'd be happy to take care of this a bit later in the summer, unless anyone else gets to it first.

jasa-acs commented 4 months ago

I don't want to bloat our release with the July 31 deadline for the CRAN issue with a bunch of things, but having this in that release could be a reasonable goal.

paciorek commented 4 months ago

Ack, ignore the weirdness that the message came from "jasa-acs". I was mistakenly signed in to that GH account.

danielturek commented 4 months ago

Addressed in PR #1475.

@paciorek In regard to your question: we previously already did handle the case of "the target node has two samplers (either two univariate or one univar and one mv) assigned to it", in which case our previous "handling" was to:

You'll read in the comments above that I changed the handling of this case, so the new behaviour is:

danielturek commented 4 months ago

Merged #1475 after tests passed.

Closing issue.