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

RFC: Remove special functions from Base #20427

Merged
merged 1 commit into from
Feb 11, 2017
Merged

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Feb 3, 2017

This PR removes bessel, hankel, airy, error, dawson, eta, zeta, digamma and its inverse, trigamma, and polygamma functions from Base as part of #19598. The dependency on openspecfun will remain until rem2pi can be written without calling into openspecfun.

Fixes #8536

@ararslan ararslan added deprecation This change introduces or involves a deprecation maths Mathematical functions labels Feb 3, 2017
bessely,
bessely0,
bessely1,
besselyx,
beta,
Copy link
Member

@JeffBezanson JeffBezanson Feb 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move beta and lbeta as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, but those are simple extensions of gamma/lgamma, so in the interest of not doing too much at once, I left those for now.

@JeffBezanson
Copy link
Member

Looks like libspecfun.a is accidentally checked in here.

@ararslan
Copy link
Member Author

ararslan commented Feb 3, 2017

Ah whoops, thanks. Will fix.

@tkelman
Copy link
Contributor

tkelman commented Feb 4, 2017

Why do we need to check in openspecfun source, couldn't we leave those functions ccalls as-is until there are pure Julia replacements? I don't see much advantage in moving a subset of the C code into this repository when the build machinery for openspecfun already exists and is pretty well-proven by now.

src/Makefile Outdated
@@ -203,12 +203,18 @@ $(BUILDDIR)/support/libsupport.a: $(SRCDIR)/support/*.h $(SRCDIR)/support/*.c
$(BUILDDIR)/support/libsupport-debug.a: $(SRCDIR)/support/*.h $(SRCDIR)/support/*.c
$(MAKE) -C $(SRCDIR)/support debug BUILDDIR='$(abspath $(BUILDDIR)/support)'

$(BUILDDIR)/specfun/libspecfun.a: $(SRCDIR)/specfun/*.h $(SRCDIR)/specfun/*.c $(BUILDDIR)/support/libsupport.a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this actually depend on libsupport?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then the makefile rule shouldn't be written as if it does

@ararslan
Copy link
Member Author

ararslan commented Feb 4, 2017

Why do we need to check in openspecfun source, couldn't we leave those functions ccalls as-is until there are pure Julia replacements? I don't see much advantage in moving a subset of the C code into this repository when the build machinery for openspecfun already exists and is pretty well-proven by now.

Fair points. The advantage of moving just rem2pi stuff into libjulia is to remove the dependency on openspecfun, since we'd then only be using it for one function, which seems silly. But I'm fine reverting all of that and just leaving openspecfun as-is if that's what's best.

@oxinabox
Copy link
Contributor

oxinabox commented Feb 4, 2017

(See also simonbyrne/libm.jl for a mostly complete implementation of the requisite functionality in Julia.)

Do you mean @simonbyrne 's 2 years stale repo, or one of the more recent works? (which have also involved @simonbyrne )

@tkelman
Copy link
Contributor

tkelman commented Feb 4, 2017

Moving C code around and getting the build system to be happy with it can be a fair amount of work, especially if the hopefully-not-too-distant plan is to replace it anyway? Openspecfun will be good to get rid of particularly since it's a Fortran library (so if it gets moved to a package where your plan is to compile it from source, you can expect a lot of users to not have a Fortran compiler installed), but it sounds like we're not all the way there yet.

@ararslan
Copy link
Member Author

ararslan commented Feb 4, 2017

@tkelman I have indeed put quite a bit of work into getting it right, and the build system is happy with it in my local tests (current CI failure is because I forgot to remove the docs, so doctests are failing), but I understand the concern.

Yes, my plan for the SpecialFunctions package is for users to compile from source unless:

  1. They're on macOS, in which case there's a Homebrew formula for openspecfun with bottles, or

  2. They're on Windows, in which case I'm hoping we can cross-compile and host some binaries on BinTray.

I think gfortran is still included in most Linux distros, so I'm not too worried about Linux users being left in the cold. Even if it isn't, I would venture a guess that most Linux users could figure out how to install gfortran and compile.

@oxinabox Yes, I meant Simon's libm.jl, which includes a mostly complete implementation of the only function we need here. It's only missing Payne-Hanek reduction for large arguments. If another package has a complete implementation, that's great.

@StefanKarpinski
Copy link
Member

I'm also fine with continuing to build and ship libopenspecfun with Julia and just moving the Julia code that uses that library into a package, which would allow rem2pi to continue using libopenspecfun. That's similar to the libRmath situation we had for a long time. Hopefully this will take a bit less time to sort out, but there's no need to get to the perfect state here.

@ararslan
Copy link
Member Author

ararslan commented Feb 6, 2017

@StefanKarpinski Yeah, Tony and I chatted about that yesterday. I think that's a good course of action, especially since SpecialFunctions.jl can then continue to use libopenspecfun without having to use BinDeps for 0.6.

@StefanKarpinski
Copy link
Member

Great. That's a plan then.

@ararslan
Copy link
Member Author

ararslan commented Feb 7, 2017

Things are good to go on this end, and the package will (hopefully) soon be registered! See JuliaLang/METADATA.jl#7859. Once that's done, this should be good to merge.

@@ -672,65 +661,6 @@ end
# Deprecate isimag (#19947).
@deprecate isimag(z::Number) iszero(real(z))

@deprecate airy(z::Number) airyai(z)
@deprecate airyx(z::Number) airyaix(z)
@deprecate airyprime(z::Number) airyaiprime(z)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these deprecations are fairly new and should stay

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How should I deprecate them if the functions they should be deprecated in favor of are no longer in Base? Something like

function airy(z::Number)
    error("airy(z) has been deprecated in favor of airyai(z) in the SpecialFunctions package.")
end

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, but I guess the deprecation targets are still exported from Base but themselves emit errors on use

:erfcx, :erfi, :dawson,
# base/special/bessel.jl
:airyai, :airyaiprime, :airybi, :airybiprime,
:besselj0, :besselj1, :bessely0, :bessely1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the package defined deprecated vectorized versions of these?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, didn't seem necessary but I can add them to the package if you think that'd be best.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we haven't had a release with them deprecated, it's only been on master, so I would do that for the sake of anyone upgrading

@ararslan
Copy link
Member Author

ararslan commented Feb 9, 2017

Per a chat with Tony, all deprecations have been moved from here to the package and symbols already deprecated here are just added to the list here that emits an error pointing to SpecialFunctions. The functions are properly deprecated in SpecialFunctions.

@tkelman tkelman added this to the 0.6.0 milestone Feb 9, 2017
signflip(m, zeta(s,z) * (-gamma(s)))
end
end

# TODO: better way to do this
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't look like these are needed any more

# the coefficients here are Float64(bernoulli[2:9])
ψ += t*w * @evalpoly(w,0.16666666666666666,-0.03333333333333333,0.023809523809523808,-0.03333333333333333,0.07575757575757576,-0.2531135531135531,1.1666666666666667,-7.092156862745098)
end

signflip(m::Number, z) = (-1+0im)^m * z
signflip(m::Integer, z) = iseven(m) ? z : -z
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only used in code that was moved?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you're right (here and above). Will remove. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh but since you were importing the Base version in the package, you'll want to add these there instead

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that's what I figured I'd do.

@tkelman
Copy link
Contributor

tkelman commented Feb 9, 2017

I might miss having erf and erfc without an import a little bit, but LGTM aside from the dead code noted above, and the post-merge TODO (JuliaLang/METADATA.jl#7859 (comment)) on the package importing for Julia versions after this merges (and up to the deprecations getting deleted, so should also verify isdefined before extending anything from Base).

We should move the PR by @oxinabox over to the package as well.

@ararslan ararslan force-pushed the aa/farewell-specialfuncs branch 2 times, most recently from 65ee782 to ff974d2 Compare February 10, 2017 03:44
@ararslan
Copy link
Member Author

32-bit AppVeyor failure seems unrelated (an error in pkg tests). Log backed up here. Worth restarting AV?

@StefanKarpinski
Copy link
Member

I don't think so, that package cloning problem happens occasionally due to bad connectivity from AppVeyor machines. The failure is after all the rest of the tests passed and the package tests passed on the other Windows build, so should be fine. As long as the Travis builds are good, you're in the clear.

@stevengj
Copy link
Member

@ararslan, could I have a collaborator invite in SpecialFunctions?

@simonbyrne
Copy link
Contributor

@stevengj I sent you an invite

@ararslan
Copy link
Member Author

@stevengj You've had a pending JuliaMath membership invitation for months now. 😉

@ararslan ararslan changed the title WIP: Remove special functions from Base RFC: Remove special functions from Base Feb 10, 2017
@ararslan
Copy link
Member Author

I'd like to get this in before something else creates a conflict and I have to rebase, causing the PR to languish in the Travis OS X queue for an eternity, so speak now or forever hold your peace. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants