-
Notifications
You must be signed in to change notification settings - Fork 73
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
Conversation
all of the values involved are integers or simple fractions so we might as well use exact equality checks to rule out issues
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.
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. |
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. |
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 ofNumeric.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:
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 inghci
thatrecip . negate . expm1 . negate $ -1000 = -0.0
. However,diff log1mexp (-1000)
still yields0.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.