Skip to content

getPosteriorLogDensity method of conjugate samplers doesn't modify target value#925

Merged
paciorek merged 4 commits into
develfrom
crossLevelFix
Oct 12, 2019
Merged

getPosteriorLogDensity method of conjugate samplers doesn't modify target value#925
paciorek merged 4 commits into
develfrom
crossLevelFix

Conversation

@danielturek
Copy link
Copy Markdown
Member

DO NOT MERGE.

Creating PR to trigger testing.

@danielturek
Copy link
Copy Markdown
Member Author

This PR has passed testing, suggesting the conjugate samplers still function correctly with these changes.

Waiting for @paciorek to determine if this fixes the problem he observed (#924) in the crossLevel sampler.

@paciorek
Copy link
Copy Markdown
Contributor

paciorek commented Aug 4, 2019

Looks good, except I'll note that even in identity link cases we write back into the model even though model[[target]] has not changed. Is it worth checking and only doing this if necessary? Or best just to always do it?

@paciorek
Copy link
Copy Markdown
Contributor

@danielturek I'd like to merge this in soon. Can you look at the identity link case and see what you think vis-a-vis my comment above?

@danielturek
Copy link
Copy Markdown
Member Author

@paciorek Apologies that this slipped by me back in August.

I've just updated the crossLevelFix branch to reflect your suggestion: the conjugate sampler will not "restore" the value of the target node, nor call model$calculate(...), when the link is "identity", in which cases they should not have been modified.

Please take a look any time, and let me know if you agree.

Assuming so, I'm fine for you to merge this in.

@paciorek paciorek merged commit 8d80b24 into devel Oct 12, 2019
@paciorek paciorek deleted the crossLevelFix branch October 12, 2019 17:25
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