JuliaStats / KernelDensity.jl

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

Tag new release? #30

Closed ranjanan closed 8 years ago

ranjanan commented 8 years ago

Tests have deprecation warnings on 0.1.2, but don't on master (Julia v0.5). Could you please tag a new release?

tkelman commented 8 years ago

0.1.2 and master are identical

$ cat KernelDensity/versions/0.1.2/sha1
48cfa6aec1acb6de62b2aee547281293a298eeef
ranjanan commented 8 years ago

Ah, I see. I got a deprecation warning the first time I did Pkg.test, because of this line. Pkg.checkout made it go away, so I posted this issue here.

WARNING: symbol is deprecated, use Symbol instead.
 in depwarn(::String, ::Symbol) at ./deprecated.jl:64
 in symbol(::String, ::Vararg{String,N}) at ./deprecated.jl:30
 in @glue(::Any) at /Users/ranjan/.julia/v0.5/KernelDensity/src/KernelDensity.jl:21
 in include_from_node1(::String) at ./loading.jl:426
 in macro expansion; at ./none:2 [inlined]
 in anonymous at ./<missing>:?
 in eval(::Module, ::Any) at ./boot.jl:234
 in process_options(::Base.JLOptions) at ./client.jl:239
 in _start() at ./client.jl:318
while loading /Users/ranjan/.julia/v0.5/KernelDensity/src/KernelDensity.jl, in expression starting on line 28
ranjanan commented 8 years ago

Interestingly, after Pkg.free and Pkg.test I'm not able to reproduce this.

ranjanan commented 8 years ago

Ah, wait, I get this warning during precompilation. That's why the first Pkg.add gave me that warning.

tkelman commented 8 years ago

Looks like it depends on whether or not you have PyPlot or Winston installed. That @glue macro has the same precompilation issue that Requires.jl does, and should probably be replaced with something that uses whatever the small piece of Plots.jl or RecipesWhatever you're supposed to use now.

ranjanan commented 8 years ago

What does this have to do with PyPlot or Winston? I didn't understand your comparison.

I think just putting @compat Symbol in that line would fix this.

tkelman commented 8 years ago

Look at what the macro's used for. https://github.com/JuliaStats/KernelDensity.jl/blob/48cfa6aec1acb6de62b2aee547281293a298eeef/src/KernelDensity.jl#L28-L29

If this package did plotting in a more modern precompilation-compatible way it wouldn't need to define that macro at all.

ranjanan commented 8 years ago

Oh okay. So, do you think a refactor of this package to use Plots instead is required?

tkelman commented 8 years ago

For it to properly be precompile-compatible (and therefore for any other packages that depend on this one to be correctly precompile-compatible), yes. But at the moment you could just fix the deprecation.

ararslan commented 8 years ago

I'd be in favor of removing the plotting stuff from here entirely. If it belongs anywhere I would think it'd be StatPlots. At the very least we could just give an example in the readme or whatever on how to plot if a plotting package is available.

tkelman commented 8 years ago

It looks like StatPlots depends on this package, so that sounds ideal.

ranjanan commented 8 years ago

@ararslan Shall I put together a PR removing plotting entirely then?

ararslan commented 8 years ago

@ranjanan Sure, if you want to then go for it. 👍 I can do it if you'd rather not.

tlnagy commented 8 years ago

Hey @ranjanan, did you have time to get that PR together?

ararslan commented 8 years ago

He did indeed: https://github.com/JuliaStats/KernelDensity.jl/pull/31 😄

ranjanan commented 8 years ago

Now that #31 has merged, can we tag a new release please?

ararslan commented 8 years ago

Sure, will do.