Skip to content

for testing only: use self=TRUE with getParents in setupMargNodes - #1630

Open
paciorek wants to merge 6 commits into
develfrom
check-setupMargNodes
Open

for testing only: use self=TRUE with getParents in setupMargNodes#1630
paciorek wants to merge 6 commits into
develfrom
check-setupMargNodes

Conversation

@paciorek

Copy link
Copy Markdown
Contributor

We are seeing an inconsistency between nimbleModel and nimble in terms of getParents returning some "self" items if they are parents of other input nodes.

This checks if using self=TRUE in with getParents in setupMargNodes changes any behavior.

If not, that suggests we could use self=TRUE in nimbleModel:::setupMargNodes to get consistent results with nimble.

@paciorek

Copy link
Copy Markdown
Contributor Author

A test in test-setupMargNodes in line 439 is failing. I think that the test as it stands has the incorrect result and that with the change to self=TRUE the result is correct, but this needs discussion. I am changing the test for now to see if other tests pass.

@paciorek

Copy link
Copy Markdown
Contributor Author

@perrydv can you look at the change to the test in line 439 of test-setupMargNodes.R in this PR and see what you think about what the correct result is? If we use self=TRUE in all cases of getParents in which self was previously not specified in setupMargNodes, then the result includes s[1] and s[2] whereas previously these were excluded from randomEffectsNodes.

@perrydv

perrydv commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

@paciorek I'm looking at this. It is admittedly a strange case in that providing only calcNodes but not any other arguments doesn't seem like a common use case. Going back to the documentation, I will try to parse what was intended and see if it makes sense to you and to this example.

If randomEffectsNodes is missing:

  1. Start with all latent nodes.
  2. If paramNodes was provided, filter to include only latent nodes that are downstream of paramNodes
  3. Filter to include only remaining nodes that satisfy one of the following conditions (only one of which will be applicable):
    a. Ancestors of dataNodes (applies only if calcNodes is missing)
    b. Ancestors or elements of calcNodes(applies only if calcNodes and paramNodes are both provided)
    c. Elements of calcNodes (applies if calcNodes was provided but paramNodes is missing)

If I am following this correctly, in the test in question step 3(c) would apply, and then s[1] and s[2] would be included in randomEffectsNodes because they are latent nodes that are elements of calcNodes. So I think that argues in favor of your correction to the test result.

I am just puzzled about whether this was a mistake when the tests were written, or the interpretation of the confusing part of the documentation is correct, or what.

The result of marginalizing with no data or given nodes at the end of a calculation would be trivial. Maybe that's why it was assumed the correct test result would not represent that. But on the other hand, the purpose of this case seems to be to follow the logic even if it is not a very smartly specified problem.

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.

2 participants