Skip to content

Commit 6d46400

Browse files
perrydvpaciorek
andauthored
Candidate fix to #1564 (but in getConditionallyIndependentSets) (#1576)
* don't mark deteministic nodes as touched during getConditionallyIndependentSets graph traversals * Add regression test for issue 1564. --------- Co-authored-by: Christopher Paciorek <paciorek@stat.berkeley.edu>
1 parent 634525f commit 6d46400

2 files changed

Lines changed: 30 additions & 2 deletions

File tree

packages/nimble/inst/CppCode/nimbleGraph.cpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -856,7 +856,8 @@ void nimbleGraph::exploreUp(vector<int> &deps,
856856
exploreDown(deps, thisParentCgraphID,
857857
isGivenVec, isLatentVec, unknownsAsGiven, recursionDepth + 1);
858858
}
859-
thisParentNode->touched = true; // Touch it now in case it was deterministic and hence not touched above
859+
// Experimental: Commenting out this line to try fixing Issue #1564
860+
// thisParentNode->touched = true; // Touch it now in case it was deterministic and hence not touched above
860861
}
861862
// myindent(recursionDepth);
862863
// std::cout<<"Exiting exploreUp from "<<thisGraphNode->name<<std::endl;
@@ -904,7 +905,9 @@ void nimbleGraph::exploreDown(vector<int> &deps,
904905
if(!isGiven) {
905906
exploreDown(deps, thisChildCgraphID,
906907
isGivenVec, isLatentVec, unknownsAsGiven, recursionDepth + 1);
907-
thisChildNode->touched = true;
908+
// Experimental: Commenting out this line to try fixing Issue #1564
909+
// Note that this touch setting would only impact !isGiven && !isStoch (because above !isGiven && isStoch is handled)
910+
// thisChildNode->touched = true;
908911
}
909912
}
910913
// myindent(recursionDepth);

packages/nimble/tests/testthat/test-setupMargNodes.R

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -439,3 +439,28 @@ test_that("setupMargNodes finds correct randomEffectsNodes based on calcNodes in
439439
expect_identical(SMN$randomEffectsNodes, c("r[1]","r[2]"))
440440
expect_identical(SMN$paramNodes, c("p[1]","p[2]"))
441441
})
442+
443+
test_that("regression tests that `getConditionallyIndependentSets` works with traversal of determ nodes", {
444+
code <- nimbleCode({
445+
for(i in 1:5) {
446+
y[i] ~ dnorm(b1*x[i] + mu[i], sd = exp(b2*x[i]+ mu2[i]))
447+
mu[i] ~ dnorm(0, tau)
448+
mu2[i] ~ dnorm(0, tau2)
449+
}
450+
b1~dflat()
451+
b2 ~dflat()
452+
tau ~ dhalfflat()
453+
tau2~dhalfflat()
454+
})
455+
m <- nimbleModel(code, data = list(y = rnorm(5)))
456+
given <- c('tau','tau2',paste0('y[', 1:5, ']'))
457+
458+
## Correct
459+
latents <- c('b1','b2',paste0('mu[', 1:5, ']'),paste0('mu2[', 1:5, ']'))
460+
expect_length(m$getConditionallyIndependentSets(nodes = latents, givenNodes = given, unknownAsGiven = TRUE), 1)
461+
462+
## In issue 1564, incorrect with different order for the latents: `mu2[1]` split out into its own set
463+
latents <- c(paste0('mu[', 1:5, ']'),paste0('mu2[', 1:5, ']'),'b1','b2')
464+
expect_length(m$getConditionallyIndependentSets(nodes = latents, givenNodes = given, unknownAsGiven = TRUE), 1)
465+
})
466+

0 commit comments

Comments
 (0)