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

Change SafePoisson and SafeNegativeBinomial to return value from rand calls #492

Open
SamuelBrand1 opened this issue Oct 10, 2024 · 2 comments
Labels

Comments

@SamuelBrand1
Copy link
Collaborator

In light of discussion here JuliaStats/Distributions.jl#1905 (comment)

We can expect the behaviour of rand(d::Distributions) to change in a way that will ameliorate edge-case numerical instability, e.g. InexactErrors. The origin of these issues as I understand it is the behaviour of Distributions.eltype, which can't always precisely predict the output type of a rand call from the type of the Distribution. This tracks the point made by @devmotion on the PR.

The simplest fix I can think of before (possible) implementation of linked PR is to either

  • make Safe... Continuous and treat integers as x.0
  • make Safe... Real valued.
@seabbs
Copy link
Collaborator

seabbs commented Dec 13, 2024

Status of this?

@SamuelBrand1
Copy link
Collaborator Author

Not sure; I think we have decisions to make dependent on how JuliaStats/Distributions.jl#1905 ends up and downstream Turing activity around construction variable info e.g. if Turing determines that a particular container for random calls should have Int eltype then we will get errors if promotion to BigInt occurs. This might happen even if eltype(dist) specifies either Int or BigInt because Turing doesn't have to decide the type of the random trace from the eltype(dist) (this has proven to be a problematic feature).

Also, from f2f with @torfjelde its a bit sub-optimal to have Union type eltypes because of the possible effect on compiler branching optimisations; we have that on main as part of the Safe distributions.

tl; dr... I'm not sure and since things are changing elsewhere I've gone a bit "wait and see".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants