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

Add isnan(::PhasePoint) #381

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add isnan(::PhasePoint) #381

wants to merge 1 commit into from

Conversation

penelopeysm
Copy link
Member

See TuringLang/Turing.jl#2389 for full context, but TLDR: if any of the values inside the PhasePoint are NaN's, we want Turing's sampling to error immediately instead of the current behaviour which is to enter an infinite loop.

This function provides a way for upstream packages such as Turing to detect this error condition.

@torfjelde
Copy link
Member

TLDR: if any of the values inside the PhasePoint are NaN's, we want Turing's sampling to error immediately instead of the current behaviour which is to enter an infinite loop.

Not sure we want this?

I guess there are two things here:

  1. Is it sensible to implement isnan for PhasePoint? Probably 🤷
  2. Is it sensible to error in Turing.jl when PhasePoint is NaN rather than just rejecting? Probably not 😕

Though I understand (2) is maybe a bit counter-intuitive, the reality is that with these random programs, you might encounter NaNs even though the code is all correct (just because we encounter particularly badly behaved parts of the space). In these cases, we want to reject the points, as they are just 0-probability choices, rather than error and break stop the sample call completely. In particular during warmup/adaptation phases where we can be quite aggressive in tuning of sampler parameters, leading to numerical issues, etc.

@sunxd3
Copy link
Member

sunxd3 commented Nov 5, 2024

I agree with Tor here, we probably don't want to error directly. Introducing some kind of guard with number of attempts should be good.

If we do decide not to error, would isnans here be still useful?

@torfjelde
Copy link
Member

Introducing some kind of guard with number of attempts should be good.

There's already a guard that warns the user, buuuuut I think in practice this isn't as useful as I was hoping when I first introduced it because IO isn't given priority and the user never ends up seeing the warning until they interrupt the computation manually 😬

@penelopeysm
Copy link
Member Author

penelopeysm commented Nov 5, 2024

I did actually see the warning when I ran it!

Screenshot 2024-11-05 at 18 08 58

Yes, if you'd rather error after N attempts (for some large N) then this PR isn't strictly needed. Unless – maybe we could say that after N attempts we can error, and if isnan(z) then add more description to the error (just to help with troubleshooting)? In that case then this would still be useful.

@penelopeysm
Copy link
Member Author

I suppose the question is whether seeing ±Inf means something different from seeing NaN.

My assumption is that NaN indicates that something is wrong and the user should look at it. But if that's not true and you can get NaN's even when doing everything right, then there's no difference (and we shouldn't include it in an error message either).

@torfjelde
Copy link
Member

My assumption is that NaN indicates that something is wrong and the user should look at it.

This is unfortunately not the case 😕 For example

julia> Inf / Inf
NaN

(got it from here: https://docs.julialang.org/en/v1/manual/integers-and-floating-point-numbers/#Special-floating-point-values)

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