Add Instance Definitions for log1p, expm1, log1pexp, and log1mexp
#111
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I, too, lack the expertise to say whether these definitions are correct and numerically precise, but they certainly pass the eyeball test. Do you know why the tests produce different results on GHC 8.6 and earlier? |
I just looked into it and I believe it to be related to https://gitlab.haskell.org/ghc/ghc/-/issues/17125. In which case, the |
|
Ah, OK. In that case, I'm not sure that we can do much besides guarding the tests behind CPP. |
|
Maybe my first call was wrong. According to its version of |
|
Okay, the versioning of This also means that I cannot cleanly use the |
RyanGlScott
reviewed
Mar 12, 2024
RyanGlScott
approved these changes
Mar 12, 2024
|
I've experienced the regression tests introduced here failing reliably, but only on x86_64-darwin (not aarch64-darwin, aarch64-linux, x86_64-linux). Sample Log: Any ideas? |
|
Good catch. I wonder if we should be defining diff --git a/tests/Regression.hs b/tests/Regression.hs
index b03266e..d212daa 100644
--- a/tests/Regression.hs
+++ b/tests/Regression.hs
@@ -143,7 +143,12 @@ issue108 diff = testGroup "issue-108" [tlog1p, texpm1, tlog1pexp, tlog1mexp] whe
-- however, zero signedness is currently not reliably propagated through some modes
-- see also https://github.com/ekmett/ad/issues/109 and https://github.com/ekmett/ad/pull/110
eq :: Double -> Double -> Bool
-eq a b = isNaN a && isNaN b || a == b
+eq a b = isNaN a && isNaN b ||
+ signum a == signum b && isInfinite a && isInfinite b ||
+ nearZero (a - b)
+
+nearZero :: Double -> Bool
+nearZero a = abs a <= 1e-12
list :: (a -> a -> Bool) -> [a] -> [a] -> Bool
list eq as bs = length as == length bs && and (zipWith eq as bs)Thoughts, @julmb? |
|
That seems like a reasonable definition of equality or distance for the
extended reals to me
…On Tue, May 28, 2024 at 6:17 PM Ryan Scott ***@***.***> wrote:
Good catch. I wonder if we should be defining eq like this instead?
diff --git a/tests/Regression.hs b/tests/Regression.hs
index b03266e..d212daa 100644--- a/tests/Regression.hs+++ b/tests/Regression.hs@@ -143,7 +143,12 @@ issue108 diff = testGroup "issue-108" [tlog1p, texpm1, tlog1pexp, tlog1mexp] whe
-- however, zero signedness is currently not reliably propagated through some modes
-- see also #109 and #110
eq :: Double -> Double -> Bool-eq a b = isNaN a && isNaN b || a == b+eq a b = isNaN a && isNaN b ||+ signum a == signum b && isInfinite a && isInfinite b ||+ nearZero (a - b)++nearZero :: Double -> Bool+nearZero a = abs a <= 1e-12
list :: (a -> a -> Bool) -> [a] -> [a] -> Bool
list eq as bs = length as == length bs && and (zipWith eq as bs)
Thoughts, @julmb <https://github.com/julmb>?
—
Reply to this email directly, view it on GitHub
<#111 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAABBQU4WLN5XBDCKNR7HQ3ZET65ZAVCNFSM6AAAAABEQKP57KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZWGE4TGOJUGQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
Ah, that is very annoying. I wonder what
This is how equality used to be handled in the tests before #110. However, in this PR we have cases like these:
So, while We probably want to measure relative error instead of absolute error, but then extra care has to be taken when the expected result is zero. In that case, we probably want to force the actual result to also be exactly zero. |
OK. Are you willing to submit a PR which updates the test suite to use relative error? |
|
I don't know when I'll have the chance to do it, but I'll try and make one in the next couple of days. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses #108. It builds on #110.
The derivatives for
log1pandexpm1are pretty much the straightforward ones.For
log1pexp, we can do some adjustments to improve numerical stability:For
log1mexp, we can do something even more clever:In the end, I added the following instance definitions for the extra floating point functions:
It would be nice if someone could double-check these for both correctness and considerations of numerical stability.
I also added some regression tests on these functions and their derivatives both for normal and extreme values where the previous default implementation were failing.