-
Notifications
You must be signed in to change notification settings - Fork 40
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
Making KernelDensity.pdf interface consistent with Distributions.pdf #122
Open
jaksle
wants to merge
18
commits into
JuliaStats:master
Choose a base branch
from
jaksle:interface-fix
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently it errs in Julia 1.0 due to |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This solves issue #120 as well as implements functionality of PR #102, but for any dimension, not only 2D matrices as in #102.
There is one breaking change:
pdf(k::UnivariateKDE, v::AbstractVector)
becomes undefined andpdf(k::InterpolateKDE, v::AbstractVector)
now tries to calculatepdf
at multi-dimensional pointv
instead of treatingv
as a collection of 1D points. I am open to discussion but I am afraid the old version is unsalvageable. There is no such method in Distributions.jl and the existence of it conflicts with implementing pdf methods for multi-dimensional KDEs. The same functionality is now available usingpdf.(k, xs)
as in Distributions.I left methods
pdf(k2d::BivariateKDE, x,y)
andpdf(k::UnivariateKDE, xs, ys)
which do not have Distributions equivalents, but are mostly harmless.Turning on broadcast is just line 15 in KernelDensity.jl file which turns all KDE objects into scalars. But, broadcast was dysfunctional in that state because there was no constant propagation, so the efficiency was atrocious. (I am not completely sure why, but it my be because functors from Interpolation.jl have custom broadcast.) The proposed solution is to extend custom broadcast to
pdf
too, interp.jl line 37, where it gets redirected to Interpolations functor broadcast. Now calculating pdf for multiple datapoints is actually even a litte faster than before.This is supposed to be a small PR only with changes absolutely necessary to fix the interface. But there are additional changes which can be made to clear the situation more:
InterpKDE
type unfortunately does not contain information about the dimension the KDE is in, so we cannot write dimension-specific methods and errors caused by inconsistent dimensions look horrible. This can be fixed leaving the interfaceInterpKDE
but making internal typeInterpKDE{N}
or something similar.kde(M::Matrix)
, i.e. that the datapoints are rows. Alas, inDistributions
datapoints are columns, inpdf
method too. MakingKernelDensity.pdf
act on rows would be insane, so we are left with very uncomfortable situation wherekde
andpdf
methods have contradictory interface. I do not fix this because that would be another breaking change.