Skip to content
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

Regression Tests Cleanup #110

Merged
merged 8 commits into from
Mar 11, 2024
Merged

Regression Tests Cleanup #110

merged 8 commits into from
Mar 11, 2024

Conversation

julmb
Copy link
Contributor

@julmb julmb commented Mar 11, 2024

I did some cleanup of the regression tests in preparation for the pull request to #108. The two notable changes are exact equality comparisons instead of abs (a - b) <= 1e-12 as well as the addition of Numeric.AD.Mode.Forward to the tests. The rest is mostly refactoring.

I wanted to also include Numeric.AD.Mode.Forward.Double to the tests. This is currently not possible due to discrepancies between the modes (#109 (comment)).

I would have liked to also compare for the proper zero signedness with something like this:

eq :: Double -> Double -> Bool
eq a b
  | isNaN a && isNaN b = True
  | isInfinite a && isInfinite b = signum a == signum b
  | a == 0 && b == 0 = isNegativeZero a == isNegativeZero b
  | otherwise = a == b

However, this too is currently not possible due to discrepancies between the modes (#109 (comment)).

It seems that depending on the mode, the sign of the zero is not reliably propagated through the differentiation combinators. For example, I implemented log1mexp = lift1 log1mexp $ recip . negate . expm1 . negate. It can be easily tested in ghci that recip . negate . expm1 . negate $ -1000 = -0.0. However, diff log1mexp (-1000) still yields 0.0.

I do not know if the ad package promises or strives to achieve proper handling of zero signedness and IEEE special values and/or consistency between the modes. Unfortunately, it looks like neither of those is currently achieved and it would be difficult and/or a lot of effort to make this happen with the current implementation. Personally, I would be okay having this merged as-is, since some tests are still better than none.

Copy link
Collaborator

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

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

Thanks, @julmb! Some minor suggestions, but otherwise this LGTM.

tests/Regression.hs Show resolved Hide resolved
tests/Regression.hs Outdated Show resolved Hide resolved
@julmb
Copy link
Contributor Author

julmb commented Mar 11, 2024

Thanks, @julmb! Some minor suggestions, but otherwise this LGTM.

Thank you very much for the quick feedback! I was not sure whether to include these comments in the code or leave this kind of stuff to GitHub issues. I will work in your suggestions and update the PR.

@RyanGlScott
Copy link
Collaborator

I was not sure whether to include these comments in the code or leave this kind of stuff to GitHub issues.

Ideally, both. GitHub issues are better suited for detailed explanations and discussions of the problems in the code, and comments provide a nice way to reference the parts of the code affected by the issues, but without needing to dump a ton of expository text inline in the comments.

@RyanGlScott RyanGlScott merged commit 2e37c7d into ekmett:master Mar 11, 2024
10 checks passed
@julmb julmb deleted the tests branch March 11, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants