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

Scope of Bessels.jl and inclusion of other special functions (e.g. gamma, airy, scorer) #27

Open
heltonmc opened this issue Aug 1, 2022 · 26 comments

Comments

@heltonmc
Copy link
Member

heltonmc commented Aug 1, 2022

EDIT: This issue started with discussion on the gamma implementation but has moved to discussing the scope of this package and inclusion of other special functions or splitting into smaller packages. Please leave comments on what functions you would like to be included here or if you would like smaller packages.

SPLIT FROM https://github.com/heltonmc/Bessels.jl/pull/26#issuecomment-1200481143

A small note: I was perusing your gamma implementation and I think there are a couple small improvements that could be made, assuming they don't cost precision somehow. For one, if I change the v = x^(0.5*x - 0.25) to v = exp((0.5*x-0.25)*log(x)) in large_gamma, I see a big speedup. And it doesn't seem like the log would make an issue, because you'll never hit that branch when x is zero.

Second, if there's a reason not to do that, then an intermediate method like this seems to be faster when x > 11.5 but it isn't too too large:

function smallish_gamma(x)
  _x = min(x, x-ceil(x-11.5))
  _g = small_gamma(_x)
  while _x < x # use the fact that gamma(x+1) == x*gamma(x)
    _g *= _x
    _x += one(_x)
  end
  _g
end

You could then branch on x < 30.0 or something instead and at least hit the large_gamma branch less often.

I haven't checked very hard to see if these tricks introduce new issues, but I wouldn't think that they do.

Originally posted by @cgeoga in https://github.com/heltonmc/Bessels.jl/issues/26#issuecomment-1200481143

@heltonmc
Copy link
Member Author

heltonmc commented Aug 1, 2022

Thank you for looking at this @cgeoga !!! Let's discuss here to try and keep the other thread focused on bessely.

I'm very happy to take PRs on this in general which we can iron out potentially a better implementation of this. As mentioned https://github.com/heltonmc/Bessels.jl/pull/26#issuecomment-1200514696 we would definitely need to test accuracy for this. The good thing about 1d functions is we can exhaustively test all the ranges. I would say for a function as important as gamma here keeping the max error below ~5 ULP would be ideal. I need to move the gamma function into its own file as well to keep it separate and add tests as well. But as mentioned hopefully the powers are faster in 1.9, though I just did a quick test on a very old 1.9 branch and found it was much slower than 1.7, so keeping track of this by julia version would also be very helpful.

@heltonmc
Copy link
Member Author

heltonmc commented Aug 1, 2022

On a quick first test it looks like the first suggestion increased the mean ULP from 0.26595 to 1.98555 and the maximum ULP from 3 to 571.0 at x=162.81202133604663.

@oscardssmith
Copy link
Member

One thing worth noting is that although these techniques won't work for Float64, for Float32, there are a lot of options opened since you can do the internals in Float64 to avoid loosing precision.

@cgeoga
Copy link
Contributor

cgeoga commented Aug 1, 2022

Makes sense about moving the issues and apologies for the clutter. Also, wow about the additional error. I assumed it would be a little bit worse, but that is certainly more than I expected.

You've mentioned these back-of-the-envelope errors in terms of ULPs before---is there some easy demo you can point me to so I can figure out how to do that kind of error testing myself? I should learn how to do this anyway, and then I can sort of pre-screen some of these ideas before posting.

@heltonmc
Copy link
Member Author

heltonmc commented Aug 1, 2022

I haven't tried it out but see Oscar's https://github.com/JuliaMath/FunctionAccuracyTests.jl.
Though in this case I just tested against SpecialFunctions.jl

import SpecialFunctions
julia> x = rand(10000)*170
julia> maximum(abs.(1 .- SpecialFunctions.gamma.(BigFloat.(x)) ./ gamma.(x)))/ eps(Float64)

@oscardssmith
Copy link
Member

If you want more details on my thought process here, my Juliacon talk was on floating point math implementation and testing. https://www.youtube.com/watch?v=-tFS4PUHqT0

@cgeoga
Copy link
Contributor

cgeoga commented Aug 1, 2022

Nice, thank you both! Will do some studying.

@juliohm
Copy link

juliohm commented Sep 9, 2022

Can you guys comment a bit more on the current gamma implementation defined in Bessels.jl? Is it ready to use? Why is it not exported? I have a package that depends on both besselk and gamma from SpecialFunctions.jl and would like to replace this dependency by Bessels.jl that is pure Julia with zero dependencies. However, I would like to understand the future plans better before making this migration.

@heltonmc
Copy link
Member Author

heltonmc commented Sep 9, 2022

The function is definitely ready to use. We use it in practically every function in this library which is why we rolled our own implementation that is much faster. It has a max ULP around 2.5-3. But it does have some limitations as it only works for real arguments and for single and double precision. So if you need it for complex or higher precision it won't work.

The reason it is not exported is because this is probably not the best package for a native gamma function implementation. There have been some discussion about moving it over to SpecialFunctions.jl but my plan was to actually create a lightweight Gamma.jl package that just has julia implementations of gamma and similar functions. So I'd be hesitant to export it here as it is likely that we will move it out of the package in the future.

@cgeoga
Copy link
Contributor

cgeoga commented Sep 9, 2022

+1 for a Gamma.jl package, that would be great.

@juliohm
Copy link

juliohm commented Sep 9, 2022

Awesome @heltonmc , thank you for the prompt reply. I am also +1 for Gamma.jl or maybe a new package that wraps all these special functions in pure Julia, maybe SpecialFuns.jl?

@oscardssmith
Copy link
Member

the argument for splitting them up is that a lot of libraries want to use gamma functions without the precompilation time of loading everything.

@juliohm
Copy link

juliohm commented Sep 9, 2022

Thank you @oscardssmith for adding the comment. Is the compilation time a real issue for Bessels.jl or is this another case of premature modularization? I feel that many of these package splits in Julia are more harmful than helpful, specially when it comes to maintenance. The collateral effect of having many packages is that some of them die after a while without maintainers. Working on the same repo with the same code style, multiple maintainers, etc. pays off in my opinion.

@oscardssmith
Copy link
Member

Well using SpecialFunctions is already .35 seconds (and most of the code is just calling C functions).

@heltonmc
Copy link
Member Author

@juliohm that was my general idea to have a higher level package that wraps all of the mathematical functions outside of Base. I'd like to see more of a canonicalization of the math functions in Julia (particularly the special functions). In my opinion there are many packages that do similar things but the quality is variable and sometimes hard to discern for new users. And they are often put together quickly for a specific application, registered, and then never maintained after…. A python user is immediately going to SciPy or mpmath and they have some sense of quality and stability which I think is a good thing.

Part of the problem with SpecialFunctions.jl is it is actually hard to find maintainers and get things merged where the scope is not so clearly defined (and maybe nobody feels any kind of authority?). One example that I think is concerning is the most recent PR and issue about the fresnel integrals which existed in a different package where a first time contributor to the language submitted a PR to add that functionality that was carefully reviewed by a primary contributor to the language but was decided that it is better in a separate package. So that contributor submitted to the other package but that package is unmaintained…. now we are point people on SpecialFuncitons.jl to an unmaintained package where the existing code base can be improved. So to your point that is definitely an issue with all of these smaller packages. Though, ideally these can be brought under a higher level package under an organization so we can continue to improve them.

So yes… what to do with gamma 🤷‍♂️ ? I’m happy for this package to have special functions but I start to become a bit worried about scope. Which is something I think SpecialFunctions.jl is not so clearly defined. Should we also include loggamma, beta, polygama, digamma, and their incomplete forms here? But I feel like then why not just start making Bessels.jl (or rename it) to just general special functions. I still think the right thing to do is try to improve the scattered special functions already registered in the general repository and improve them and then bring them under some higher organization and repository to standardize in some way. For example, I’ve been spending some time trying to improve Struve.jl so that hopefully we can have several similar quality libraries to piece together.

But I think I agree with your overall sentiment…. Though maybe I was hoping to improve the already existing registered with very defined scope that a higher level package can reexport. So new users can be a little more confident in the quality of the package they are using and having an easy interface with a single package. Though say if you wanted just the gamma function, you can then just depend on that package which has a very defined scope and very lightweight.

Really appreciate your thoughts and definitely welcome to other suggestions!

@juliohm
Copy link

juliohm commented Sep 10, 2022 via email

@heltonmc
Copy link
Member Author

Yes - and that is definitely the most important thing to consider. We should probably be a bit more mindful on the best thing for users here. It's probably best to only have one dependency in their tree so that it is easier to manage versioning etc.

But I don't think it's wise to have tiny packages like you mentioned. I'd like to have packages that have closely related families of functions. For example, here I plan to give bessel functions, modified bessel functions, spherical bessel functions, airy functions, and hopefully soon kelvin functions. The amount of source code for all of that is getting quite large and the amount of time it takes to test each function is also increasing. Then a gamma package can contain all the gamma and related functions I mentioned earlier. I'm actually trying to do some convincing over at Struve.jl to include related functions as well but we'll see...

But getting back to your original question. The gamma function here is quite good but will probably be moved to a Gamma.jl package. Then can create a top level package that re-exports all of the special functions.

Name suggestions for a top level package are most welcome! As well as anyone who would like to help with that!

@juliohm
Copy link

juliohm commented Sep 10, 2022 via email

@heltonmc
Copy link
Member Author

O wow thank you!! I've been locally commenting out test groups not to run which is definitely a bit of a hack.

@juliohm
Copy link

juliohm commented Sep 12, 2022

@heltonmc any timeline for the Gamma.jl package? I am looking forward to drop SpecialFunctions.jl from my dependencies and favor Bessels.jl + Gamma.jl. In the meantime, can I safely depend on Besses.jl and use Bessels.gamma explicitly even if it is not part of the public API? Are you planning to release a minor release when it is moved to Gamma.jl?

@heltonmc
Copy link
Member Author

heltonmc commented Sep 12, 2022

You can explicitly depend on Bessels.gamma yes (it's an explicitly tested function anyway). I will definitely follow the guidelines I lay out at https://github.com/JuliaMath/Bessels.jl/blob/master/NEWS.md#changelog on when to increment minor versions and include Bessels.gamma as an unoffical exported function. When we move gamma out of this package we will increment the minor version (e.g., v0.2 -> v0.3) and make very big mention of it in the NEWS.md file. I'll comment here as well.

The timeline is tricky as I want to be as fair as possible giving credit to all the original authors of julia code in SpecialFunctions.jl that worked on gamma like functions (there's a few functions I'd like to move over along with the implementation here). To me the best way to do this is by preserving all of the git history for the necessary files. Of course I am not a maintainer or contributor explicitly at SpecialFunctions.jl so wouldn't want to step on any toes. I'll work to see if I can get this done this week.... but I really don't want to make any promises there.

@heltonmc
Copy link
Member Author

And just another comment so it's clear for anyone using gamma. If you call gamma with Bessels.gamma even if we move the function to another package that call will still work in your code so you don't really have to worry about any breaking changes (the API for gamma is very stable). Every function in this package relies on gamma so it will also be loaded within the module either by a using Gamma or with the function defined within the package as it is now.

Now is that good practice...... probably not 🤷‍♂️ but this package will always depend heavily on gamma so that functionality will always be there.

@heltonmc heltonmc changed the title gamma function implementation Scope of Bessels.jl and inclusion of other special functions (e.g. gamma, airy, scorer) Sep 27, 2022
@heltonmc
Copy link
Member Author

I've updated the title of this issue as it has evolved into the scope of this project itself rather than the actual gamma implementation.

I think I've become a bit more persuaded by @juliohm and some others who have reached out to have a more centralized package instead of it being fragmentated. As of now @time_imports using Bessels takes about 10 milliseconds on my computer which I think makes this package even though it has a large amount of code fairly lightweight for users and won't contribute to longer package load times. However, the TTFX will be longer as most of the time is on code generation.

Another thing I'm starting to realize is testing might be better if all related functions with overlapping code were under the same folder that were tested simultaneously by CI. For example, there is sometimes a chain of function calls like gamma->besselk->airy->inhomog. airy. But in some domains, besselk may need to call the airy implementation. So I'm thinking it might make more sense if there is any overlapping code that they share the same project to make sure any changes are rigorously tested within the scope of all dependent special functions. Of course in my case gamma is always independent and could be separate....

Therefore, I might suggest that this package be home to any functions listed in Chapter 9-11 of NIST (https://dlmf.nist.gov/) which is also how mpmath (https://mpmath.org/doc/current/functions/index.html) groups them though they also include Chapter 12 and some of the special cases of hypergeomtric functions.

I think I may make a discourse announcement for this package and maybe some discussion can continue there...

Thanks everyone for the thoughts!

@juliohm
Copy link

juliohm commented Sep 28, 2022

Having a single umbrella for these functions will save you a lot of maintenance headache for sure @heltonmc. Looking forward to seeing it become the next generation special functions in Julia 🚀

@juliohm
Copy link

juliohm commented Sep 30, 2022

@heltonmc I am trying to purge SpecialFunctions.jl from my indirect dependencies...

Do we have the functionality to replace these three functions from this core package already?

https://github.com/JuliaGraphics/ColorVectorSpace.jl/blob/8ec1d3549cab5064be2f09b8bb8de6f502c9c2ba/src/ColorVectorSpace.jl#L26

I can submit a PR to ColorVectorSpace.jl to remove SpecialFunctions.jl and add Bessels.jl instead.

@heltonmc
Copy link
Member Author

heltonmc commented Sep 30, 2022

I actually thought SpecialFunctions already had a loggamma implementation but it looks like it’s just for complex values. I am on vacation until next week so can’t tell exactly what is going on. I think having a good loggamma implementation here is important as it is needed. And I believe logabsgamma can be called from that.

Is ‘lfact’ deprecated? Is that just the logarithm of the factorial? That could also be easily defined from the loggamma algorithm… seems like an important function. I actually had an implementation of loggamma early on that I’ll try to track down again. I’ll try to look into that next week.

But to answer your question only gamma is supported right now and the other ones should really be defined from a dedicated loggamma implementation.

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

No branches or pull requests

4 participants