Skip to content

Conversation

@oscardssmith
Copy link
Contributor

Through some clever use of Remez.jl (and some testing to better limit where the fallback is appropriate), we can remove almost all of the branches from the Float64 implementation and add a similar (but slightly faster) Float32 version.

This PR needs some CI tests added before merging, but I believe the algorithm is correct at this point.

Through some clever use of Remez.jl (and some testing to better limit where the fallback is appropriate), we can remove almost all of the branches from the Float64 implementation and add a similar (but slightly faster) Float32 version.
@oscardssmith
Copy link
Contributor Author

oscardssmith commented Jul 7, 2025

@devmotion can you clarify what tests you want added? We already seem to have tests for Float32. I have just pushed a commit significantly strengthening the Float32 and Float64 tests (testing to ~3 ULPs rather than just vaguely close to correct)

@oscardssmith
Copy link
Contributor Author

@devmotion do you understand the failing CI? I've looked and there doesn't seem to be a connection between failures in logsumexp gradients with log1pmx...

@tpapp
Copy link
Collaborator

tpapp commented Jul 8, 2025

I can't retrieve the logs, I am assuming that this is some Github glitch, will check back later.

@oscardssmith
Copy link
Contributor Author

Test Summary: | Pass  Total  Time
logmxp1       |  106    106  0.0s
ForwardDiff: Test Failed at /home/runner/work/LogExpFunctions.jl/LogExpFunctions.jl/test/basicfuns.jl:390
  Expression: ForwardDiff.gradient(logsumexp, x) ≈ ∇x
   Evaluated: [NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN, NaN] ≈ [0.0, 0.05446345555425227, 0.04025051057488777, 0.08412871027400531, 0.04497453835046083, 0.07308833432487137, 0.23864370877722943, 0.09204946765522504, 0.07333530177596569, 0.29906597271310204]

Stacktrace:
  [1] record(ts::Test.DefaultTestSet, t::Union{Test.Error, Test.Fail}; print_result::Bool)
    @ Test /opt/hostedtoolcache/julia/1.11.5/x64/share/julia/stdlib/v1.11/Test/src/Test.jl:1111
  [2] record
    @ /opt/hostedtoolcache/julia/1.11.5/x64/share/julia/stdlib/v1.11/Test/src/Test.jl:1104 [inlined]
  [3] do_test(result::Test.ExecutionResult, orig_expr::Any)
    @ Test /opt/hostedtoolcache/julia/1.11.5/x64/share/julia/stdlib/v1.11/Test/src/Test.jl:712
  [4] macro expansion
    @ /opt/hostedtoolcache/julia/1.11.5/x64/share/julia/stdlib/v1.11/Test/src/Test.jl:679 [inlined]
  [5] macro expansion
    @ ~/work/LogExpFunctions.jl/LogExpFunctions.jl/test/basicfuns.jl:390 [inlined]
  [6] macro expansion
    @ /opt/hostedtoolcache/julia/1.11.5/x64/share/julia/stdlib/v1.11/Test/src/Test.jl:1704 [inlined]
  [7] macro expansion
    @ ~/work/LogExpFunctions.jl/LogExpFunctions.jl/test/basicfuns.jl:382 [inlined]
  [8] macro expansion
    @ /opt/hostedtoolcache/julia/1.11.5/x64/share/julia/stdlib/v1.11/Test/src/Test.jl:1704 [inlined]
  [9] top-level scope
    @ ~/work/LogExpFunctions.jl/LogExpFunctions.jl/test/basicfuns.jl:239
 [10] include(fname::String)
    @ Main ./sysimg.jl:38
 [11] top-level scope
    @ ~/work/LogExpFunctions.jl/LogExpFunctions.jl/test/runtests.jl:15
 [12] include(fname::String)
    @ Main ./sysimg.jl:38
 [13] top-level scope
    @ none:6
 [14] eval
    @ ./boot.jl:430 [inlined]
 [15] exec_options(opts::Base.JLOptions)
    @ Base ./client.jl:296
 [16] _start()
    @ Base ./client.jl:531
Test Summary: | Pass  Fail  Total   Time
logsumexp     |  168     1    169  12.0s
  ForwardDiff |    1     1      2   2.6s
ERROR: LoadError: Some tests did not pass: 168 passed, 1 failed, 0 errored, 0 broken.
in expression starting at /home/runner/work/LogExpFunctions.jl/LogExpFunctions.jl/test/basicfuns.jl:238
in expression starting at /home/runner/work/LogExpFunctions.jl/LogExpFunctions.jl/test/runtests.jl:15
Package LogExpFunctions errored during testing
Error: Process completed with exit code 1.

@tpapp
Copy link
Collaborator

tpapp commented Jul 9, 2025

MWE for failure:

using ForwardDiff
using LogExpFunctions
wrap(x) = LogExpFunctions._logsumexp_onepass_op(x[1], x[2], x[3])[2]
x = [-Inf, -Inf, 0.0]
wrap(x)                         # 1.0
ForwardDiff.gradient(wrap, x)   # NaN NaN NaN

though I a not sure what why we expected this to be finite / well-behaved in the first place. Is this a "derivative defined in the limit" argument?

@tpapp
Copy link
Collaborator

tpapp commented Jul 9, 2025

The CI runs above are using ForwardDiff v1.0.1. The test runs in #95 are using ForwardDiff v0.10.22.

I don't understand why.

@tpapp
Copy link
Collaborator

tpapp commented Jul 9, 2025

See #96 for an update that reproduces this. It is unrelated to your PR, but I think it would be great to fix it first, merge, and then get back to this.

@tpapp
Copy link
Collaborator

tpapp commented Jul 14, 2025

Please merge master and re-run CI, I hope that works now and then I will merge.

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!

@tpapp
Copy link
Collaborator

tpapp commented Jul 14, 2025

@devmotion: can you please clarify what other tests would be needed? I think that tightening the bounds on the previous ones is fine.

@oscardssmith oscardssmith changed the title improve log1pmx, add `Float32 implimentation improve log1pmx, add Float32 implimentation Jul 14, 2025
@tpapp
Copy link
Collaborator

tpapp commented Jul 17, 2025

Since it is still not clear to me what extra tests @devmotion is asking for, I am merging this since I think it is valuable feature addition. Extra tests can always be incorporated later as necessary.

@tpapp tpapp merged commit 66ddb31 into JuliaStats:master Jul 17, 2025
4 checks passed
@oscardssmith oscardssmith deleted the patch-2 branch July 17, 2025 12:19
@devmotion
Copy link
Member

I was mainly referring to the first comment:

This PR needs some CI tests added before merging, but I believe the algorithm is correct at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants