Skip to content

fix accuracy of logcosh(::Union{Float16, Float32, Float64}) #101

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Aug 17, 2025
Merged

fix accuracy of logcosh(::Union{Float16, Float32, Float64}) #101

merged 11 commits into from
Aug 17, 2025

Conversation

@nsajko
Copy link
Contributor

@nsajko nsajko commented Aug 2, 2025

Fixes #100

@nsajko
Copy link
Contributor Author

nsajko commented Aug 2, 2025

Plots of the error in ULPs after this change

logcosh(::Float32):

f32

logcosh(::Float64):

f64

The plotting happens after downsampling (which just takes the maximum value), which itself happens after smoothing (which also just takes the maximum, i.e., a sliding window maximum). Thus the downsampling and smoothing basically just remove the downward spikes of the noise, preserving the maximum error at the region around each point.

tpapp
tpapp approved these changes Aug 6, 2025
View reviewed changes
Copy link
Collaborator

@tpapp tpapp left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

nsajko added 2 commits August 6, 2025 16:04
@nsajko
update ULPError
03037ba
@nsajko
update ULPError
4fc1b9d 
oscardssmith has been reviewing the `ULPError` code on
JuliaLang/julia#59087

So sync the code with the changes there.
@tpapp tpapp requested a review from devmotion August 8, 2025 06:50
devmotion
devmotion reviewed Aug 8, 2025
View reviewed changes
nsajko added 4 commits August 9, 2025 04:42
@nsajko
ulp_error_maximum: use Fix1
12ed7ce 
As suggested by devmotion on PR #99.
@nsajko
test suite: use ulp_error_maximum instead of for loop
714e08a 
As suggested by devmotion on PR #99.
@nsajko
ULPError: adjust whitespace style as per devmotion
297586b
@nsajko
ulp_error: delete unnecessary method static parameters
b754b40 
Specialization on the callable argument types should happen anyway.
@nsajko nsajko mentioned this pull request Aug 14, 2025
Merged
@nsajko
Copy link
Contributor Author

nsajko commented Aug 14, 2025

bump

devmotion
devmotion reviewed Aug 14, 2025
View reviewed changes
Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

The PR looks good, I just wonder if we could cover Float16 as well?

@nsajko nsajko changed the title fix accuracy of logcosh(::Union{Float32, Float64}) fix accuracy of logcosh(::Union{Float16, Float32, Float64}) Aug 16, 2025
@tpapp
Copy link
Collaborator

tpapp commented Aug 16, 2025

@nsajko: thanks for adding Float16. As far as I am concerned this is ready merging now.

@tpapp tpapp merged commit b88d17d into JuliaStats:master Aug 17, 2025
4 checks passed
@tpapp tpapp mentioned this pull request Aug 17, 2025
Open
@nsajko nsajko deleted the logcosh_accuracy branch August 17, 2025 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@devmotion devmotion devmotion approved these changes

@tpapp tpapp tpapp approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

logcosh(x) accuracy vanishes around the zero, at x = 0

3 participants

@nsajko @tpapp @devmotion