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

Merged
merged 14 commits into from
Jun 28, 2024
Merged

Check model by default #2218

merged 14 commits into from
Jun 28, 2024

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
Member

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.

@torfjelde
Copy link
Member Author

torfjelde commented Jun 18, 2024

I think the IS tests are all failiing because of previous changes which lead to evaluation of the model twice, buuut I don't understand why these weren't failiing before o.O These should have been failiing for a while.

EDIT: ah no ofc, it's failiing because the initial model check alters the random seed...

@torfjelde
Copy link
Member Author

Think the random seeds have just been tuned carefully for those MH inference tests so because of the slight alterations in the RNG due to the model check run, they're now failiing.

I've tried to just provide decent initial params to sample which seems to have done the trick.

@yebai yebai merged commit cbd5d79 into master Jun 28, 2024
57 checks passed
@yebai yebai deleted the torfjelde/check-model branch June 28, 2024 21:05
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.

3 participants