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

Use JuliaLibm log functions in Main.Math. #24750

Merged
merged 4 commits into from
Feb 11, 2018
Merged

Conversation

pkofod
Copy link
Contributor

@pkofod pkofod commented Nov 24, 2017

These should be just as precise as openlibm (own experience and https://tpapp.github.io/post/log1p/ ) but I will ofc provide solid numbers on this.

@stevengj raised the point that it's concerning that log1p seems a bit slower for small values as per the blog post above, and these are the values where you really want log1p. I'll benchmark this further and if it is the case, my idea is to do openlibm port for the small x part and tang's for the larger x's.

Should the old ones (those in a module) be deprecated? They've been mentioned in the docstrings, so...

@ararslan ararslan added the maths Mathematical functions label Nov 24, 2017
@pkofod
Copy link
Contributor Author

pkofod commented Nov 27, 2017

Float64
If we look at log it seems to be a straight improvement in precision at the evaluated points.
julialibm_improvement

The same thing goes for timings
relative_time

So log for Float64 seems like a pareto improvement.

Float32
Precision
julialibm_improvement_float32

Speed
relative_time_float32

Slightly slower around 1.

cc: @simonbyrne
code appendix: https://gist.github.com/pkofod/0e361627401c894135ac139c71e538ea

@pkofod
Copy link
Contributor Author

pkofod commented Nov 28, 2017

Should the JuliaLibm-module part of it be deprecated as it's been mentioned in the docstrings?

@ararslan
Copy link
Member

I doubt anyone has been relying on Base.Math.JuliaLibm so I don't see a need to deprecate it.

@pkofod
Copy link
Contributor Author

pkofod commented Nov 29, 2017

I doubt anyone has been relying on Base.Math.JuliaLibm so I don't see a need to deprecate it.

Alright, I was only wondering since it's actually mentioned in the docstring of log, but fine by me. I should just remove it then?

@vchuravy
Copy link
Member

I would say it is better to be conservative and deprecate them properly.

@pkofod
Copy link
Contributor Author

pkofod commented Nov 29, 2017

I would say it is better to be conservative and deprecate them properly.

Is there a special syntax that lets you deprecate unexported functions from modules?

@vchuravy
Copy link
Member

@eval Math.JuliaLibm begin
    Base.@deprecate ...
end

@pkofod pkofod force-pushed the logs branch 3 times, most recently from bd9bc4c to cc47850 Compare November 30, 2017 10:42
@@ -2168,6 +2168,11 @@ end

@deprecate merge!(repo::LibGit2.GitRepo, args...; kwargs...) LibGit2.merge!(repo, args...; kwargs...)

# Remember to delete the module when removing this
Copy link
Member

Choose a reason for hiding this comment

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

You could just move the whole definition here:

@eval Base.Math module JuliaLibm
    Base.@deprecate log Base.log
end

Copy link
Contributor Author

@pkofod pkofod Feb 6, 2018

Choose a reason for hiding this comment

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

Will fix!

@pkofod
Copy link
Contributor Author

pkofod commented Feb 7, 2018

Errors seem unrelated?

@pkofod
Copy link
Contributor Author

pkofod commented Feb 11, 2018

Shameless bump. This is approved by two people, and tests seem to pass, so I am wondering if there is more I need to do.

@StefanKarpinski StefanKarpinski merged commit 11c08ad into JuliaLang:master Feb 11, 2018
@pkofod pkofod deleted the logs branch February 11, 2018 20:39
@pkofod pkofod mentioned this pull request Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants