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

[paciorek]

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]

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]

@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]

@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.

[paciorek]

@perrydv regarding your suggested revision to the help text, you say:

"c. Elements of calcNodes (applies if calcNodes was provided but paramNodes is missing)"

but in the code I see:

        reNodesDefault <- intersect(reNodesDefault,
                                    calcNodes)
        reNodesDefault <- intersect(reNodesDefault,
                                    model$getParents(calcNodes, upstream=TRUE, stochOnly = TRUE))

which seems to indicate that the randomEffectsNodes need to be in both the calcNodes and in the parents of calcNodes (with self=FALSE), which would seem to be an impossibility except for those situations we've been discussing where the one input node is also the parent of another input node.

Thoughts?

[perrydv]

@paciorek I see the confusion and don't have a good answer. I agree that in the old behavior this code would result in only calcNodes that are parents of other calcNodes. I am not sure if the text (which I tried to re-parse) was from reading this code or described the original intent and then the code deviated from that. It is more evidence that there is not really clear correct behavior to a default in this poorly specific case. Apparently the idea was that if only calcNodes are given, then the default for random effects nodes will be any latent nodes that are given in the calcNodes. That seems as simple and reasonable as we'll get. And if so then it appears the last setdiff there should be removed. Is that what you're thinking?