-
Notifications
You must be signed in to change notification settings - Fork 22
improve log1pmx, add Float32 implimentation
#94
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
Conversation
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.
|
@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) |
|
@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... |
|
I can't retrieve the logs, I am assuming that this is some Github glitch, will check back later. |
|
|
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 NaNthough 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? |
|
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. |
|
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. |
|
Please merge master and re-run CI, I hope that works now and then I will merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
|
@devmotion: can you please clarify what other tests would be needed? I think that tightening the bounds on the previous ones is fine. |
log1pmx, add `Float32 implimentationlog1pmx, add Float32 implimentation
|
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. |
|
I was mainly referring to the first comment:
|
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.