search

JuliaStats / KernelDensity.jl

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

Support Interpolations v0.9 #61

Closed JLTastet closed 5 years ago

JLTastet commented 5 years ago

This PR fixes #60 and removes deprecations warnings introduced with Interpolations 0.9. I have also added a few trivial regression tests.

I have tentatively added version bounds for Interpolations to avoid unwanted breakage in the future. So we will need to manually bump the upper bound after having checked that the new version works (and until Interpolations publishes a stable release). The downside is that we now require Interpolations >=0.9.

This PR does not break the KernelDensity API itself.

Let me know if you have comments or suggestions (e.g. if you want to continue supporting older versions of Interpolations). I may have forgotten to update the code in some places, so it would be nice if someone who is familiar with KernelDensity could double-check that.

simonbyrne commented 5 years ago

Thanks!

andreasnoack commented 5 years ago

The upper bound is causing some problems for other packages, see https://github.com/JuliaLang/METADATA.jl/pull/19114, so it would be great if you would reconsider this. In exchange, I'll increase the number of packages tested by CIBot such that a future breakage of Interpolations is more likely to be detected.

JLTastet commented 5 years ago

@andreasnoack Do you think we should raise the upper bound, or remove it altogether?

andreasnoack commented 5 years ago

I think either completely removing it or making it a major release upper bound would be best.

JLTastet commented 5 years ago

IIRC, without the upper bound I used to have a similar issue to the one linked above. I could not rollback Interpolations without causing an ambiguity during dependency resolution.

I think either completely removing it or making it a major release upper bound would be best.

Interpolations is still a 0.x release so according to semver any minor release could in principle be breaking. Since KernelDensity is also at 0.x, maybe this is fine if it occasionally breaks after an update. Users should maintain a properly version-controlled Project.toml anyway, so they can rollback in case they need it. This is what I started doing after encountering this issue.

The long-term solution will obviously be Interpolations tagging a stable release. In the meantime, I think we should choose whichever solution minimizes the risk of future breakage. I would welcome any help from someone who is familiar with Pkg3 here.

As a workaround, I will raise the upper bound to 0.11-, so that users can continue to get work done. 0.10 is not advertised as breaking so this should be fine (not tested, but CI should catch any issues should they arise).

andreasnoack commented 5 years ago

Let me just reiterate that having upper bounds on packages cause problems. It can introduce situations where there isn't a unique solution for the package resolution, it might be so hard to find the solution that the resolver finds another solution, or the actual solution ends up being something different from what people expect. As minimum, please don't introduce upper bounds as part of a bugfix release. I.e. if you realize it is required then please add the upper bound to existing releases within the minor release.

The better approach, in my opinion, is that we increase the number of tested packages in CIBot such that we don't miss a breakage again as we did in this case here.