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

Remove quadgk and friends from Base #19741

Merged
merged 5 commits into from
Dec 29, 2016

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Dec 28, 2016

This PR removes quadgk as well as the Base.QuadGK submodule, now that QuadGK.jl is its own registered package. The approach taken here is mirrored after the approach taken for removing the primality functions.

cc @stevengj and @tkelman

@ararslan
Copy link
Member Author

If I recall, @vtjnash had expressed concern over not having quadgk available to test inference. Let me know if there are any suitable substitutes that can be used in the tests. Alternatively, I've allowed edits from maintainers, so any maintainer is free to add replacement test code here as they see fit.

@tkelman
Copy link
Contributor

tkelman commented Dec 28, 2016

The package will get tested nightly on PkgEval. I don't think I've ever seen the quadgk test fail myself, but maybe that only happens early in big inference refactorings. Since those are relatively rare, we should make it easier to run PkgEval on-demand for that kind of PR, like we do with nanosoldier for benchmarking. There's some work underway to make that possible.

@vtjnash
Copy link
Member

vtjnash commented Dec 28, 2016

The package will get tested nightly on PkgEval. I don't think I've ever seen the quadgk test fail myself, but maybe that only happens early in big inference refactorings

It tests one aspect of convergence speed. Without specific tuning in inference, this code can take anywhere from several days (or weeks?) to infer, vs. computing the same answer in a fraction of a second.

@JeffBezanson
Copy link
Member

It's true that this is a good inference test case, but good test cases might be found anywhere in the ecosystem. I don't think that can be a factor in deciding where code lives. A good approach would be either PkgEval, or making a reduced version of this to copy into the inference test suite. After all, it's also possible the quadgk code would be updated to become a less-good inference test case.

@StefanKarpinski StefanKarpinski added this to the 0.6.0 milestone Dec 29, 2016
@tkelman tkelman merged commit dbbd7e5 into JuliaLang:master Dec 29, 2016
@ararslan ararslan deleted the aa/farewell-quadgk branch December 29, 2016 19:16
@jarvist
Copy link

jarvist commented May 9, 2017

On 0.5 I was using QuadGK and everything was hunky dory, but then,

julia> using QuadGK
julia> quadgk
WARNING: both QuadGK and Base export "quadgk"; uses of it in module Main must be qualified
ERROR: UndefVarError: quadgk not defined

Fixed by importing the quadgk function explicitly.

julia> import QuadGK.quadgk
julia> quadgk
quadgk (generic function with 3 methods)

Just thought I'd leave this here as a quick fix for anyone else who suddenly had their code break on 0.5 -> 0.6!

Some kind of guidance in the error message would be useful, particularly if these errors will become the new normal as core Julia slims down.

@ararslan
Copy link
Member Author

ararslan commented May 9, 2017

Hey @jarvist, thanks for the report. Issues with particular packages should be reported on the package's repo. If you could open an issue at https://github.com/JuliaMath/QuadGK.jl, that would be great.

@jarvist
Copy link

jarvist commented May 10, 2017

OK, can do - but I think the key issue is that the deprecation warning:

julia> quadgk()
ERROR: quadgk() has been moved to the package QuadGK.jl.
Run Pkg.add("QuadGK") to install QuadGK on Julia v0.6 and later, and then run `using QuadGK`.

Is misleading, as:

julia> using QuadGK

julia> quadgk()
WARNING: both QuadGK and Base export "quadgk"; uses of it in module Main must be qualified
ERROR: UndefVarError: quadgk not defined

I assume that's because it's clashing with the deprecation warning itself?

@ararslan
Copy link
Member Author

The handling of the exporting is an issue with the package.

@JeffBezanson
Copy link
Member

Yes it looks like it's clashing with the deprecation definition. Unfortunate, but we can probably only fix this completely when the deprecation is removed.

@stevengj
Copy link
Member

The deprecation message could be changed to:

Run Pkg.add("QuadGK") to install QuadGK on Julia v0.6 and later, and then run `import QuadGK: quadgk`.

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.

7 participants