-
Notifications
You must be signed in to change notification settings - Fork 22
add logabstanh
#86
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
add logabstanh
#86
Conversation
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.
Thank you! Can you add tests and update the docs? Probably should also add a ChainRules definition.
src/basicfuns.jl
Outdated
| The implementation ensures `logabstanh(-x) = logabstanh(x)`. | ||
| """ | ||
| function logabstanh(x::Real) | ||
| return log1p(-2/((exp(2abs(x))+1))) |
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.
As indicated by the special cases close to 0 for Float32 and Float64, maybe a safer default would be
| return log1p(-2/((exp(2abs(x))+1))) | |
| return log(abs(tanh(x))) |
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.
I think the version I wrote stays more accurate overall but I'll do a test
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.
I don't think it's possible to do such an analysis? You can't realistically test any subtype of Real.
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.
for any AbstractFloat log(tanh(x)) will be inaccurate for large x since tanh(x) will be very close to 1.
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
|
Something missing here? Or can this be merged ? |
|
|
Should I re-open and rebase to add Float32 and Float64 fast-paths? |
|
It might be simpler to just revert 39127b6 instead of rebasing. For a start I mean. |
|
@oscardssmith: choose whatever is most convenient for you, but having these methods would be very welcome. The new testing framework should be sufficient for tests. |
|
can you reopen? I seem to not have permission to. |
|
Unfortunately I am unable, Github says "that these commits are already merged." |
|
Yeah, all of them are part of the merged PR. |
|
ah. Will re-create then. |
as requested by @c05 on slack. This has 15 ULP for Float64, and 5 ULP for Float32.