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

Check model by default #2218

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Check model by default #2218

wants to merge 5 commits into from

Conversation

torfjelde
Copy link
Member

@torfjelde torfjelde commented May 8, 2024

We now have the ability to perform some rudimentary model checks (TuringLang/DynamicPPL.jl#540), and so we should probably perform a few of these at every call to sample with an inference alg.

This PR adds these checks by default. This means that now we will give informative errors if either of the following occurs:

We can also perform certain checks depending on the sampler, e.g. error early and informatively if the user attempts to use gradient based samplers for discrete parameters.

One question: Do we make these checks error by default or just warn the user? I'm preferential to the erroring, since these checks can always be turned off.

@torfjelde
Copy link
Member Author

@yebai @devmotion

@sunxd3
Copy link
Collaborator

sunxd3 commented May 8, 2024

If I read it correctly, we are using https://github.com/TuringLang/DynamicPPL.jl/blob/5347acd278146449a943e3dee647d0ff9893b189/src/debug_utils.jl#L240 to track varname, then we are rely on ==.

Should we consider x[1] and x[1:2] being a clash (and even weirder subsume cases). Also, after transitioning to Accessors, we lost some generality in equality testing. For instance, x[:] concretized on x = [1, 2, 3] is !== from x[1:3] (but it was == before).

This is not a PR blocking concern, because the PR is clearly try to catch true negative.

@torfjelde
Copy link
Member Author

Should we consider x[1] and x[1:2] being a clash (and even weirder subsume cases). Also, after transitioning to Accessors, we lost some generality in equality testing. For instance, x[:] concretized on x = [1, 2, 3] is !== from x[1:3] (but it was == before).

We indeed should! But yes, this is something we should deal with in DPPL, not in this PR.

But yeah, you're very correct that we should probably check subsumes rather than equality 👍

@torfjelde
Copy link
Member Author

This requires #2225 as some of the fixes are present there.

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.

None yet

2 participants