JuliaStats / KernelDensity.jl

Kernel density estimators for Julia
Other
177 stars 40 forks source link

update to v0.4 tuple syntax using @compat macro #14

Closed sebastiang closed 9 years ago

sebastiang commented 9 years ago

While trying to wander down the long road of getting Gadfly working in v0.4, I tried to update KernelDensity to use the new tuple syntax. Most of this is mechanical, and required only adding the Compat package. But this pull request does NOT result in a running package. there are still errors running the tests. In particular, type inference fails in a surprising way trying to evaluate kernel_dist with a tuple of distributions in its first argument.

Where previously the code was dispatching on ::Type{(Dx,Dy)}, the two obvious options both fail. One is ::Type{Tuple{Dx,Dy}} but this doesn't seem quite right, and indeed doesn't even compile. The other is to take the tuple as a value, _::Tuple{Dx,Dy}, but while this compiles, methods that ought to satisfy the pattern don't. You can see the failures by including test/bivariate.jl

JeffBezanson commented 9 years ago

Type{(Dx,Dy)} should become Type{Tuple{Dx,Dy}}, not Tuple{Dx,Dy} as you have. You dropped one set of brackets.

sebastiang commented 9 years ago

Agreed, but was trying several different types of dispatch here. I’ll see if I can make your other suggestion work.

sebastiang commented 9 years ago

This pull request has been updated. The internal function kernel_dist meant for bivariate distribution construction now dispatches using the type of the tuple, i.e. Tuple{Normal, Normal} instead of a tuple of the types (Normal, Normal). Uglier, but an additional method was added to allow the later form. Unfortunately that's not a great long-term option in case someone wants to add another kernel_dist with a tuple argument to do something with other special kinds of distributions.

simonbyrne commented 9 years ago

Fantastic, thanks!