Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix new Gibbs sampler for cases where only some variables need to be linked #2401

Open
mhauru opened this issue Nov 29, 2024 · 2 comments
Open

Comments

@mhauru
Copy link
Member

mhauru commented Nov 29, 2024

See

# TODO(mhauru) The above might run into trouble if some variables are linked and others
for description of the problem and a possible solution.

This relies on DynamicPPL gaining a method for link!! and invlink!! that takes a VarName rather than a sampler or a space.

The bug that this causes is only hit when doing things that the old Gibbs sampler never allowed in the first place, so I don't think implementing this should hold back merging #2328.

@torfjelde
Copy link
Member

This relies on DynamicPPL gaining a method for link!! and invlink!! that takes a VarName rather than a sampler or a space.

Is this meant to link only that particular variable and return the linked value or is it meant to link only that subset of the varinfo? Because the latter we sort of want to avoid as this requires runtime checks + it was only really needed currently because of the way we implemented the Gibbs sampler.

IIUC we can just have the global varinfo in #2328 always be constrained, and then we just invlink the linked varialbes in the local varinfo before updating the global one?

@mhauru
Copy link
Member Author

mhauru commented Dec 2, 2024

The idea was that this would BangBang modify the VarInfo, changing the linkage status of a variable, not just return the value. Can you help me understand in more detail why this is undesirable?

IIUC we can just have the global varinfo in #2328 always be constrained, and then we just invlink the linked varialbes in the local varinfo before updating the global one?

That would work as well, yes. I thought matching the linkage status between previous and new VarInfo was cleaner and more robust (e.g. some weird future sampler wants to have some variables linked and others not linked, no problem) than having to do unnecessary link/invlink at every iteration. But if that is problematic then indeed this can be used instead.

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

No branches or pull requests

2 participants