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 #1241; correct the behavior of logpdf(d::Dirichlet, x) #1242

Merged
merged 1 commit into from
Dec 20, 2020
Merged

Fix #1241; correct the behavior of logpdf(d::Dirichlet, x) #1242

merged 1 commit into from
Dec 20, 2020

Conversation

yurivish
Copy link
Contributor

@yurivish yurivish commented Dec 18, 2020

This fixes pdf(d::Dirchlet, x) at the boundary of and outside of the support, and replaces hard-coded Float64 values with more general types.

…at the boundary of and outside of the support, and replace hard-coded Float64 values with more general types
@codecov-io
Copy link

codecov-io commented Dec 18, 2020

Codecov Report

Merging #1242 (f694b19) into master (530b9e0) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1242      +/-   ##
==========================================
+ Coverage   81.81%   81.83%   +0.02%     
==========================================
  Files         117      117              
  Lines        6587     6590       +3     
==========================================
+ Hits         5389     5393       +4     
+ Misses       1198     1197       -1     
Impacted Files Coverage Δ
src/multivariate/dirichlet.jl 66.50% <100.00%> (+0.95%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 530b9e0...f694b19. Read the comment docs.

@mschauer
Copy link
Member

I doubt one can get it really perfect as function of n argument vectors

julia> mylogpdf(Dirichlet([1, 1.0]), [1.0 + sqrt(eps()), 0.])
-Inf

julia> mylogpdf(Dirichlet([1, 1.0]), [1.0 + 0.5sqrt(eps()), 0.])
0.0

@yurivish
Copy link
Contributor Author

yurivish commented Dec 18, 2020

That seems to be due to a bug in insupport:

julia> insupport(Dirichlet([1.0, 1.0]), [1.0 + 0.5sqrt(eps()), 0.])
true

In general the summing of valid weights may result in a total greater than 1.0 due to floating-point error, though, as you point out.

In your specific example there is a single weight above 1.0 which could be detected by insupport, which already checks for individual weights below zero.

@andreasnoack
Copy link
Member

Should insupport be adjusted as part of this PR?

@mschauer
Copy link
Member

I think not, if there are any later improvements to insupport they likely require no changes to the PR, and improving insupport is tricky without going to an proper n-1 dimensional representation of probability vectors anyway

@andreasnoack andreasnoack merged commit 2892421 into JuliaStats:master Dec 20, 2020
@devmotion
Copy link
Member

devmotion commented Dec 22, 2020

I suspect this PR, and in particular the type promotions, broke DistributionsAD: https://github.com/TuringLang/DistributionsAD.jl/pull/143/checks?check_run_id=1592162436

Maybe the issues are fixed by #1243.

@mschauer
Copy link
Member

The error no method matching Float64(::Tracker.TrackedReal{Float64}) suggest
raw = rand(rng, float(eltype(p))) in https://github.com/JuliaStats/Distributions.jl/pull/1236/files#diff-532294b375dcec6c10dc02b6b34da3851007768391e96b6d02c2a87eb74a1626R76 could also be a culprit?

@devmotion
Copy link
Member

This change is not released yet, is it?

@mschauer
Copy link
Member

Hm, no, but perhaps it will be a problem?

@devmotion
Copy link
Member

The error occurs only for Dirichlet whereas the other change affected DiscreteNonParametric. Additionally, we do not differentiate through rand in DistributionsAD (well, apart from normal distributions...) and I checked that rand(float(eltype(Tracker.params(x))) works fine.

@mschauer
Copy link
Member

Thanks for checking.

@devmotion
Copy link
Member

It took me a while but in the end I figured out that the latest release of NNlib is responsible for the test failures: FluxML/NNlib.jl#251

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.

5 participants